-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(nuxt): Add server config to root folder #13583
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
size-limit report 📦
|
6cd63cb
to
ea95e37
Compare
This indeed would be helpful as I have a project which a merge of a laravel a nuxt project. Nuxt project is in the folder
This indeed |
@nandi95 If I understand you correctly, you are defining a curstom |
I only meant to say that I define a |
if (moduleOptions.debug) { | ||
// eslint-disable-next-line no-console | ||
console.log( | ||
'[Sentry] Using your `sentry.server.config` file for the server-side Sentry configuration. In case you have a `public/instrument.server` file, it will be ignored.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(...) it will be ignored
This reads a bit confusing, which of the two files are being ignored?
I think we could check here for the existence of the file and log explicitly if that file is getting ignored, wdyt?
Also, we are ignoring the public file here but users will still potentially --import
it. This log makes it sound like it's completely ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this log should be split up.
1st part (public): can we log a warning if we detect the file in public
instead of the first part of this message? Otherwise, I don't think we need to bother users with it, right?
To the second part: Can we instead print the exact import command? Something like
"Make sure to add --import ./.output/sentry.server.config.mjs
to your NODE_OPTIONS
env variable"
if (moduleOptions.debug) { | ||
// eslint-disable-next-line no-console | ||
console.log( | ||
'[Sentry] Successfully added the content of the `sentry.server.config` file as `instrument-sentry.mjs` to the `.output/server` directory', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: Maybe we can use destination
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Other than the console log, this looks good to me!
We can follow up with the e2e tests after this is merged, given we hit some road blocks there
it('should return the server file with .ts extension if it exists', () => { | ||
vi.spyOn(fs, 'existsSync').mockImplementation(filePath => { | ||
return !(filePath instanceof URL) && filePath.includes('sentry.server.config.ts'); | ||
}); | ||
|
||
const result = findDefaultSdkInitFile('server'); | ||
expect(result).toBe('sentry.server.config.ts'); | ||
}); | ||
|
||
it('should return the client file with .mjs extension if it exists', () => { | ||
vi.spyOn(fs, 'existsSync').mockImplementation(filePath => { | ||
return !(filePath instanceof URL) && filePath.includes('sentry.client.config.mjs'); | ||
}); | ||
|
||
const result = findDefaultSdkInitFile('client'); | ||
expect(result).toBe('sentry.client.config.mjs'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: For some test extra credit (feel free to disregard) - we could use an it.each
to test against all file types we search for.
Hello! Thank you for the work you have done! Could you please clarify, should we somehow modify the command for running the project on server in package.json? I see that now we can put sentry.server.config.ts in the root folder, but did not find any instructions on how to modify starting command. Previously, it was required to paste this:
If i put my config file in the root folder and deleted instrument.server.mjs from public folder, should I modify these commands as well? |
@wanderowski This PR is not released yet and more elaborate docs on this are still coming for the official docs page. The file can be added like this: "start": "node --import ./.output/server/sentry.server.config.mjs .output/server/index.mjs", As it only affects the server, it can only be added to the However, it could be that your environment variables from the |
@s1gr1d Thanks! |
Makes it possible to include a
sentry.server.config.ts
file in the root folder alongsidesentry.client.config.ts
. Currently, it has to be added in thepublic
folder which is not 100% ideal.