-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(nextjs): Avoid Edge build warning from OpenTelemetry process.argv0
#18759
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
|
|
||
| // This key must match `@sentry/opentelemetry`'s `SENTRY_SCOPES_CONTEXT_KEY`. | ||
| // We duplicate it here so the Edge bundle does not need to import the full `@sentry/opentelemetry` package. | ||
| const SENTRY_SCOPES_CONTEXT_KEY = createContextKey('sentry_scopes'); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| // &&= | ||
| out = out.replace(/([A-Za-z_$][\w$]*(?:\[[^\]]+\]|\.[A-Za-z_$][\w$]*)+)\s*&&=\s*([^;]+);/g, (_m, left, right) => { | ||
| return `${left} = ${left} && ${right};`; | ||
| }); |
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.
Regex transformation breaks operator precedence with ternary expressions
Medium Severity
The downlevelLogicalAssignmentsPlugin regex transformation produces semantically different code when the right-hand side contains a ternary expression. For example, obj.foo ??= cond ? a : b; becomes obj.foo = obj.foo ?? cond ? a : b;. Due to operator precedence (?? binds tighter than ?:), this is parsed as (obj.foo ?? cond) ? a : b rather than obj.foo ?? (cond ? a : b). The transformation needs to wrap the captured right value in parentheses: ${left} = ${left} ?? (${right}); to preserve the original semantics.
logaretm
left a comment
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.
is this a temporary measure till they fix it or...
LGTM anyways to unblock us
|
Don't think they'll fix this, there's nothing to fix from their POV. I don't think they have edge support on their agenda. I think we need to leave some comments in the vendored functions we imported from the other sentry packages to reflect that any changes in them also have to be done in our vercel-edge sdk. |
|
|
||
| // This key must match `@sentry/opentelemetry`'s `SENTRY_SCOPES_CONTEXT_KEY`. | ||
| // We duplicate it here so the Edge bundle does not need to import the full `@sentry/opentelemetry` package. | ||
| const SENTRY_SCOPES_CONTEXT_KEY = createContextKey('sentry_scopes'); |
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.
Context key mismatch breaks scope lookup in Edge
High Severity
The createContextKey function from @opentelemetry/api uses Symbol() (not Symbol.for()), so each call creates a unique symbol. The local SENTRY_SCOPES_CONTEXT_KEY created here will be a different symbol than the one created in @sentry/opentelemetry, despite having the same description. This means getScopesFromContext will always return undefined because the context was set using @sentry/opentelemetry's symbol but is being read with a different symbol. The isolation scope mutation in lines 138-140 will silently fail, breaking scope isolation for Edge runtime requests.
Additional Locations (1)
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
andreiborza
left a comment
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.
Not pretty but alas..
Fix Next.js Edge build warnings caused by OpenTelemetry’s process.argv0 usage by ensuring OTEL deps are bundled/rewritten in @sentry/vercel-edge and by removing unnecessary @sentry/opentelemetry imports from @sentry/nextjs Edge/shared utilities.
closes #18755