-
-
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
fix(nextjs): Include nextjs config's basePath on urlPrefix #3922
Conversation
Hi, @jemmyphan. Thanks for the contribution! This generally looks good, but reviewing it pointed out to me a few places where I think the existing test code didn't work well. So sit tight - I'm going to push those changes through, and then you can rebase this on top of that. I'll keep you posted. |
In the process of reviewing #3922, I realized that there was some cleanup work I could do to make that PR work better. Specifically: - `withSentryConfig` had an incorrect type for one of the arguments to the `userNextConfig` function. - The mock for that same argument was missing its outer layer, and therefore wasn't really what it said it was. - In real life, the `config` property of `buildContext` is equal to the combination of next's default config and the user-provided config. The mock wasn't reflecting this. - That PR was having to compensate for the fact that the mocks for build context were static rather than dynamic, even though those test cases especially illustrate the ways in which the value of the mock needs to change depending on other values in the test. Where necessary, the build context is now calculated as part of the test.
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.
Okay! I made your tests match my changes when I was doing them, just to make sure everything worked, so I've included that, along with some other small updates, as suggestions below. If you commit those, pull, rebase onto current master
, and then force push, I think we should end up where we want to be!
71bc1e2
to
fa8e735
Compare
thanks for the review @lobsterkatie, |
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.
LGTM! Thanks!
When using
SentryWebpackPlugin
as part of the nextjs SDK, uploaded sourcemap names point to the wrong path if the nextjsbasePath
setting is used. This makes it so that theurlPrefix
value includesbasePath
, if set.