feat(nuxt): Configure sentry in external config #12681
Conversation
packages/nuxt/package.json
Outdated
| "import": "./build/esm/index.client.js", | ||
| "require": "./build/cjs/index.client.js" | ||
| }, | ||
| "node": "./build/cjs/index.server.js" |
There was a problem hiding this comment.
m: this should be both esm and cjs
There was a problem hiding this comment.
I actually I'd double check this too to see if ESM works well on the server 😅
packages/nuxt/src/module.ts
Outdated
| } | ||
| }, | ||
| }); | ||
|
|
||
| function findDefaultSdkInitFile(type: 'server' | 'client'): string | undefined { | ||
| const possibleFileExtensions = ['ts', 'js', 'mjs', 'cjs', 'mts']; |
There was a problem hiding this comment.
| const possibleFileExtensions = ['ts', 'js', 'mjs', 'cjs', 'mts']; | |
| const possibleFileExtensions = ['ts', 'js', 'mjs', 'cjs', 'mts', 'cts']; |
packages/nuxt/src/module.ts
Outdated
| if (resolver) { | ||
| addPlugin(resolver.resolve('runtime/plugins/sentry.client.js')); | ||
| addPlugin(resolver.resolve('./runtime/plugins/sentry.client')); | ||
| addServerPlugin(resolver.resolve('./runtime/plugins/sentry.server')); |
There was a problem hiding this comment.
m: It seems like this PR has some server-side logic with the addServerPlugin and the server side sdk being exposed.
To ease reviewing, could we remove these changes from this PR and add it later when we look at the server-side stuff?
packages/nuxt/src/module.ts
Outdated
| addImportStatement(nuxtApp.rootComponent, buildSdkInitFileImportSnippet(pathToClientInit)); | ||
| } catch (err) { | ||
| // eslint-disable-next-line no-console | ||
| console.error(err); |
There was a problem hiding this comment.
m: we should add some notes that this is coming from sentry and we encountered this error while trying to add imports.
| defaults: {}, | ||
| setup(_moduleOptions, _nuxt) { | ||
| setup(_moduleOptions, nuxt) { | ||
| const resolver: Resolver = createResolver(import.meta.url); |
There was a problem hiding this comment.
l: Can we double check to see if this works with CJS? How does this get transpiled? We currently have build/module/module.cjs emitted.
There was a problem hiding this comment.
For the module, I am using the nuxt module builder which transpiles the files exactly how they should be imported in Nuxt. The module.cjs looks like this:
module.exports = function(...args) {
return import('./module.mjs').then(m => m.default.call(this, ...args))
}
const _meta = module.exports.meta = require('./module.json')
module.exports.getMeta = () => Promise.resolve(_meta)
packages/nuxt/src/common/types.ts
Outdated
| @@ -1,3 +1,3 @@ | |||
| import type { init } from '@sentry/vue'; | |||
|
|
|||
| export type SentryVueOptions = Parameters<typeof init>[0] & object; | |||
| export type SentryVueOptions = Omit<Parameters<typeof init>[0] & object, 'app'>; | |||
There was a problem hiding this comment.
l: Would be useful to leave a comment about why this we have a difference between vue init and nuxt init here.
packages/nuxt/src/common/snippets.ts
Outdated
| /** Returns an import snippet */ | ||
| export function buildSdkInitFileImportSnippet(filePath: string): string { | ||
| const pathToPosix = (originalPath: string): string => { | ||
| return originalPath.split(path.sep).join(path.posix.sep); |
There was a problem hiding this comment.
l: why elect for the inline function pathToPosix over
const posixPath = filePath.split(path.sep).join(path.posix.sep);
return `import "${posixPath}"`There was a problem hiding this comment.
True 😅 I had two functions first and moved pathToPosix inside the other one.
packages/nuxt/src/common/snippets.ts
Outdated
| fs.writeFileSync(filePath, output, 'utf8'); | ||
| } catch (err) { | ||
| // eslint-disable-next-line no-console | ||
| console.error(`Error writing file: ${err}`); |
There was a problem hiding this comment.
m: Can we indicate that this comes from sentry? Same with console.error below.
packages/nuxt/src/common/snippets.ts
Outdated
| export function addImportStatement(filePath: string, importStatement: string): void { | ||
| try { | ||
| const data = fs.readFileSync(filePath, 'utf8'); | ||
| const scriptIndex = data.indexOf('<script setup>'); |
There was a problem hiding this comment.
m: Let's extract <script setup> into a constant, and add a comment about why we are looking for this.
packages/nuxt/src/common/snippets.ts
Outdated
| const scriptIndex = data.indexOf('<script setup>'); | ||
|
|
||
| // Insert the import statement after the script tag | ||
| const output = [ |
There was a problem hiding this comment.
l: I think doing string.replace is a bit easier to understand then indexOf + constructing a new output via slice
const output = data.replace(SETUP_TAG, `${SETUP_TAG}\n${importStatement}`);| } | ||
| } catch (err) { | ||
| // eslint-disable-next-line no-console | ||
| console.error(`[Sentry] Error reading file at ${filePath}: ${err}`); |
There was a problem hiding this comment.
l: we should have emit a error message if we fail to find <script setup> in the read file, and make this separate from the error message we get from fs.readFileSync failing.
Adding tests for #12681 --------- Co-authored-by: Abhijeet Prasad <[email protected]>
|
|
||
| export default makeNPMConfigVariants( | ||
| makeBaseNPMConfig({ | ||
| entrypoints: ['src/index.client.ts', 'src/client/index.ts'], |
There was a problem hiding this comment.
do we need both of these entry points? What's the difference between them?
| @@ -1 +1 @@ | |||
| export {}; | |||
| export * from './client'; | |||
There was a problem hiding this comment.
TODO: This will also have to export the types from the server, but we can do that in a follow up :)
There was a problem hiding this comment.
So far there is no server-side instrumentation. But when this gets added I will also add the types here 👍🏻
| }, | ||
| "./package.json": "./package.json" | ||
| "./module": { | ||
| "types": "./build/module/types.d.ts", |
There was a problem hiding this comment.
TODO: Will we have to do the same as for solid here, where types for a submodule export have to be in the root? 😬 no need to adjust this in this PR, but have a look at it in a follow up, and maybe sync with @andreiborza on this :)
To be able to differentiate between a browser/client execution context, sentry is initialized in an external config file. An import statement in
nuxt-root.vueis added which loads this config file.Nuxt tracking issue: #9095