Skip to content
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

fix(nextjs): Fix devserver CORS blockage when assetPrefix is defined #9766

Merged
merged 3 commits into from Dec 7, 2023

Conversation

lforst
Copy link
Member

@lforst lforst commented Dec 6, 2023

Fixes #8727

When assetPrefix is defined the webpack dev server will live on that prefixed domain.

The SDK may attach tracing headers to the HMR server when the asset prefix is something like http://cdn.localhost:3000 because we match on "localhost". Because this is then a different origin we run into CORS issues with the dev server.

This PR resolves that by special casing the tracePropagationTargets when Next.js is in dev mode. This is a bit of a hack for now.

Once we get to #9764 we can remove the hack again.

@lforst lforst requested a review from Lms24 December 6, 2023 12:47
@@ -113,7 +113,16 @@ function addClientIntegrations(options: BrowserOptions): void {
if (hasTracingEnabled(options)) {
const defaultBrowserTracingIntegration = new BrowserTracing({
// eslint-disable-next-line deprecation/deprecation
tracingOrigins: [...defaultRequestInstrumentationOptions.tracingOrigins, /^(api\/)/],
tracingOrigins:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can/should we use tracePropagationTargets here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that would be breaking if people configured tracingOrigins themselves because the tracePropagationTargets would gain precedence?

// In case both are specified, tracePropagationTargets takes precedence

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm yeah could be. let's leave it as is then!

@Lms24
Copy link
Member

Lms24 commented Dec 6, 2023

What happens if users manualyl set TPTs? our fix for dev mode would no longer be applied, right?

@lforst
Copy link
Member Author

lforst commented Dec 7, 2023

What happens if users manualyl set TPTs? our fix for dev mode would no longer be applied, right?

Yeah correct. This is a tricky one. The only way we could make this sorta bulletproof is to add a new option to BrowserTracing to filter out certain urls but I would like to avoid this until we land #9764. Do you have an idea on what we could do? Ofc we could map and transform tPTs or tracingOrigins but that seems messy.

@lforst lforst merged commit 59d5554 into develop Dec 7, 2023
56 checks passed
@lforst lforst deleted the lforst-fix-dev-server-cors-with-localhost-subdomain branch December 7, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Next.js CDN (App-router) withSentryConfig in Dev mode broke client side routing
2 participants