Improve @next/plugin-sentry support and expose additional to plugin hooks #18272
Conversation
kamilogorek
requested review from
ijjk,
lfades,
Timer and
timneutkens
as code owners
| * TODO(kamil): Unify SDK configuration options. | ||
| * Workaround for callbacks, integrations and any unserializable config options. | ||
| **/ | ||
| exports.serverConfig = {} |
There was a problem hiding this comment.
The runtime configuration approach is not needed if you compile the configuration values into the bundle (prefixing using NEXT_PUBLIC_. However keep in mind that we shouldn't do that for the getRelease part given it will cause every single build to invalidate the webpack cache.
There was a problem hiding this comment.
Yes, but there's no way to provide additional configuration to Sentry.init without that approach, as NEXT_PUBLIC_ supports only serializable values.
| const { serverRuntimeConfig = {}, publicRuntimeConfig = {} } = | ||
| getConfig() || {} | ||
| const runtimeConfig = | ||
| serverRuntimeConfig.sentry || publicRuntimeConfig.sentry || {} |
There was a problem hiding this comment.
next/config should not be used. This is not compatible with Next.js' SSG mode and only exists for legacy reasons.
| const { publicRuntimeConfig = {} } = getConfig() || {} | ||
| const runtimeConfig = publicRuntimeConfig.sentry || {} |
There was a problem hiding this comment.
next/config should not be used. This is not compatible with Next.js' SSG mode and only exists for legacy reasons.
There was a problem hiding this comment.
@Timer What do you suggest to replace it?
| const { serverRuntimeConfig = {}, publicRuntimeConfig = {} } = | ||
| getConfig() || {} |
There was a problem hiding this comment.
next/config should not be used. This is not compatible with Next.js' SSG mode and only exists for legacy reasons.
| serverRuntimeConfig: { | ||
| sentry: { | ||
| type: 'server', | ||
| }, | ||
| }, | ||
| // Sentry.init config for client-side code (and fallback for server-side) | ||
| // can accept only serializeable values. For more granular control see below. | ||
| publicRuntimeConfig: { | ||
| sentry: { | ||
| type: 'client', | ||
| }, | ||
| }, |
There was a problem hiding this comment.
This should be moved to your own sentry.config.js file or similar.
| "SENTRY_DSN", | ||
| "SENTRY_RELEASE" | ||
| ] | ||
| "required-env": [] |
There was a problem hiding this comment.
Why are there no longer any required envs?
There was a problem hiding this comment.
See the PR description.
There was a problem hiding this comment.
We should fix the bug with the key please instead of bypassing it!
There was a problem hiding this comment.
I totally agree. This PR is the effect of PoC work, and it was necessary to go around this limitation for demo purposes.
|
@Mike-Koech reported your account as spammer. |
required-envis currently removed, as it doesn't respectNEXT_PUBLIC_presence, so despite providingNEXT_PUBLIC_SENTRY_DSNit would still complain that it's not there, unless{ env: { NEXT_PUBLIC_SENTRY_DSN } }would be manually provided innext.config.js.