Uh oh!
There was an error while loading. Please reload this page.
feat(nuxt): Add option autoInjectServerSentry (no default import()) - #14553
Conversation
❌ 5 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
b8caf04 to
d435ce2CompareUh oh!
There was an error while loading. Please reload this page.
| if (serverConfigFile) { | ||
| if (moduleOptions.dynamicImportForServerEntry === false) { | ||
| if (moduleOptions.autoInjectServerSentry !== 'experimental_dynamic-import') { |
There was a problem hiding this comment.
m: What if this option is undefined (i.e. people add --import)? Do we still need to inject the top level import?
There was a problem hiding this comment.
Only when the server config file is added and resolved properly (which is done in the rollup plugin with the dynamic import()), this is not needed.
In the other cases (user provided --import or top-level import), the file is not resolved properly and the import of the sentry release injection file is missing (leads to an error). By adding the import of the server config in the plugin template, the imports are resolved correctly.
However, it does not actually import the file and initialize the SDK a second time. So this is just for resolving. But I can add a comment for this.
There was a problem hiding this comment.
ah maybe I mixed something up here. Feel free to add a comment but disregard my comment otherwise :)
Uh oh!
There was an error while loading. Please reload this page.
| /** | ||
| * Extracts the filename from a path. | ||
| */ | ||
| export function getFilenameFromPath(path: string): string | null { |
There was a problem hiding this comment.
l: any reason to use this over path.basename?
There was a problem hiding this comment.
Because it's not only a path, it's the full node command like node whatever/path/index.mjs, but I can change the function signature to make it more clear.
There was a problem hiding this comment.
afaict, path.basename handles such a scenario:
but no strong feelings. If we stick with this implementation, let's just make sure we document why we need it instead of using the builtin :)
There was a problem hiding this comment.
Yeah, it generally works but my tests where I checked for a Windows-style string and for a string like this node ./server/, path.basename did not work.
Uh oh!
There was an error while loading. Please reload this page.
As injecting the dynamic
import()came with a bunch of challenges related to how the build output looks like, this default is changed to make users use the--importCLI flag.This PR basically reverts this: #13958