-
-
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): Adding experimental_basicServerTracing
option to Nuxt module
#13643
Conversation
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.
should we add a note about this to the readme?
Correct me if I'm wrong but the way this option is defined here, it will not be properly typed and supported in the user's language server right? Additionally, I would make the name a) a bit more dangerous sounding b) actually name it after what it does, because right now it sounds very good, even though it is a workaround. E.g. |
size-limit report 📦
|
packages/nuxt/src/common/types.ts
Outdated
* | ||
* @default false | ||
*/ | ||
simplifiedDeployment?: boolean; |
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.
🧵 name bikeshedding :D
Some ideas:
basicTracingOnlyDeployment
noImportFlagDeployment
noImportFlagEnvironment
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.
What do you think about a workaround_
prefix?
workaround_injectSentryConfigIntoServer
workaround_basicTracingWithServerConfigInject
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 have no strong feelings either way! IMHO the question is, do we want to describe what the flag does, or when you need it? Describing what it does is obviously technically correct, but maybe makes it harder to understand when you would/should actually use this. I'll happily defer to you and other folks on the team about the name for this! We can also start with something like experimental_XXXX
- we can still promote it to a non-experimental name later, if needed/wanted!
@lforst This option is added to the module types and TS the language server picks that up And I will change the name, wasn't so sure about it anyway so thanks for the suggestions 🙌 |
* | ||
* @default false | ||
*/ | ||
experimental_basicServerTracing?: boolean; |
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.
just a reminder to also update the PR title accordingly ;)
simplifiedDeployment
option to Nuxt moduleexperimental_basicServerTracing
option to Nuxt module
Enabling this option will import the Sentry server config at the top of the server entry file. This can be used when adding the node option
--import
does not work. This however only comes with limited tracing functionality.Example Usage: