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 |
s1gr1d
force-pushed
the
sig/import-flag-default
branch
from
b8caf04 to
d435ce2
Compare
dev-packages/e2e-tests/test-applications/nuxt-3-dynamic-import/nuxt.config.ts
Outdated
Show resolved
Hide resolved
| @@ -54,15 +58,15 @@ export default defineNuxtModule<ModuleOptions>({ | |||
| const serverConfigFile = findDefaultSdkInitFile('server'); | |||
|
|
|||
| 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 :)
packages/nuxt/src/vite/utils.ts
Outdated
| /** | ||
| * 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.
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