Uh oh!
There was an error while loading. Please reload this page.
feat(nuxt): Add Rollup plugin to wrap server entry with import() - #13945
Conversation
lforst
left a comment
There was a problem hiding this comment.
First pass. Generally looks good to me!
| ); | ||
| } | ||
| function wrapEntryWithDynamicImport(resolvedSentryConfigPath: string): InputPluginOption { |
There was a problem hiding this comment.
nit: If a function is only called in one place, and the calling function is already not that big, I always consider inlining it.
There was a problem hiding this comment.
I considered this function as the rollup plugin so I didn't want to inline it in the push(). However, I could add a JSDoc here for the plugin.
| if (source === 'import-in-the-middle/hook.mjs') { | ||
| return { id: source, moduleSideEffects: true, external: true }; | ||
| } |
There was a problem hiding this comment.
Did this end up working? If so, we should probably add a comment what this is doing here. Somebody coming across this is probably like "what the hell is this?".
There was a problem hiding this comment.
Yes, that prevented the "Cannot find module" error in Netlify. I added a comment
| moduleInfo.moduleSideEffects = true; | ||
| const exportedFunctions = moduleInfo.exportedBindings?.['.']; |
There was a problem hiding this comment.
l: Quick comment what the '.' is about?
There was a problem hiding this comment.
According to the rollup docs it's the "path of from". As the . entry includes the exports from the file I figured the dot means exactly that: The exports from this file 🤔
There was a problem hiding this comment.
Yup, I know but I think we should add a comment about that 😄
| const exportedFunctions = moduleInfo.exportedBindings?.['.']; | ||
| // checks are needed to prevent multiple attachment of the suffix | ||
| return containsSuffix(source) || containsSuffix(resolution.id) |
There was a problem hiding this comment.
Aren't we already checking for containsSuffix(source) at the top of the hook?
There was a problem hiding this comment.
Yes, for the source. I changed the code a bit so there is only the check for resolution.id at this point.
| `export function ${currFunctionName}(...args) {\n` + | ||
| ` return import(${JSON.stringify(entryId)}).then((res) => res.${currFunctionName}(...args));\n` + | ||
| '}\n', | ||
| ), |
There was a problem hiding this comment.
I would gravitate woards making this function async, awaiting the import, and calling it with .call(this, args), just so that the this is preserved in case it is passed. This is usually important when monkepatching.
| * | ||
| * Only exported for testing. | ||
| */ | ||
| export function stripQueryPart(url: string): string { |
There was a problem hiding this comment.
nit: Maybe we can make this function name a bit more representative of what it does.
There was a problem hiding this comment.
Renamed it to removeSentryQueryFromPath
| .split(',') | ||
| .filter(param => param !== '' && param !== 'default') | ||
| // sanitize | ||
| .map((str: string) => str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')) |
There was a problem hiding this comment.
What cases are we sanitizing for here?
There was a problem hiding this comment.
This function gets the functions from a file to re-export them. Theoretically, another rollup plugin could inject code here. As discussed in person, this is code a user can control so it is unlikely to be used by attackers but I'll keep it with a small comment - just to be safe.
b5d568c to
0a7cd3cCompare0a7cd3c to
5f4c53dCompareBREAKING CHANGE: The `--import` flag must not be added anymore. If it is still set, the server-side will be initialized twice and this leads to unexpected errors. --- First merge this: #13945 Part of this: #13943 This PR makes it the default to include a rollup plugin that wraps the server entry file with a dynamic import (`import()`). This is a replacement for the node `--import` CLI flag. If you still want to manually add the CLI flag you can use this option in the `nuxt.config.ts` file: ```js sentry: { dynamicImportForServerEntry: false, } ``` (option name is up for discussion)
Feature Issue: #13943
Adds a Rollup plugin to wrap the server entry with
import()to load it after Sentry was initialized.The plugin is not yet in use (will do this in another PR - see linked issue above)