-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(nuxt): Add warning when Netlify build is discovered #13868
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -69,6 +69,17 @@ export default defineNuxtModule<ModuleOptions>({ | |||||
if (serverConfigFile && serverConfigFile.includes('.server.config')) { | ||||||
addServerConfigToBuild(moduleOptions, nuxt, nitro, serverConfigFile); | ||||||
|
||||||
consoleSandbox(() => { | ||||||
const serverDir = nitro.options.output.serverDir; | ||||||
|
||||||
if (serverDir.includes('.netlify')) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe, just maybe, it's more robust to check for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would only show the warning in the Netlify console, right? I think it's also useful to have this warning locally when people choose to use the netlify build preset. But I can add a |
||||||
// eslint-disable-next-line no-console | ||||||
console.warn( | ||||||
'[Sentry] Warning: The Sentry SDK discovered a Netlify build. The server-side Sentry support with ESM is experimental and may not work as expected. Please check out the docs for how to use Sentry on different deployment providers: https://docs.sentry.io/platforms/javascript/guides/nuxt/deployment-provider-setup/', | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. idk, maybe:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am no English major but I think you have support "for" something and not "with" something. It's less about the word "ESM". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lizokm hit us with the wisdom There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's "for" :) You're right @lforst, it's "support for" you could say "with the support of..." but that's a different meaning. |
||||||
); | ||||||
} | ||||||
}); | ||||||
|
||||||
if (moduleOptions.experimental_basicServerTracing) { | ||||||
addSentryTopImport(moduleOptions, nitro); | ||||||
} else { | ||||||
|
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 would also be great if we could detect when users actually set up everything correctly for the respective platform and not show the warning in that case.
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.
Hmm...one thing we could do is getting the
NODE_OPTIONS
variable and check if the file path is correct.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's a good idea.