Skip to content

fix(nextjs): correct withSentryConfig return type #6677

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

Closed
wants to merge 1 commit into from

Conversation

seanparmelee
Copy link

In #6291, withSentryConfig was updated to always return a function so this PR updates the return value type to no longer include NextConfigObject.

@lforst lforst closed this Jan 7, 2023
@lforst
Copy link
Contributor

lforst commented Jan 7, 2023

Hi, thanks for opening this PR. The return type was intentionally kept broad because we might want to change this again in the future without introducing a breaking change.

@seanparmelee
Copy link
Author

Thanks for the consideration. FWIW, #6291 was a bit of a breaking change for us because we were expecting withSentryConfig to return an object since we're passing in an object and TS didn't catch it because it thinks an object can still be returned.

@lforst
Copy link
Contributor

lforst commented Jan 7, 2023

TS should complain if you don't handle the case where an object is returned. Technically this was not a breaking change.

@seanparmelee
Copy link
Author

seanparmelee commented Jan 7, 2023

It might be helpful if I provide a bit more background on how we encountered this.
We're using next-transpile-modules similar to this:

// next.config.js
const withTM = require('next-transpile-modules')([...]);
const { withSentryConfig } = require('@sentry/nextjs');

...

module.exports = withTM(
  withSentryConfig({
    ...
  }, {...})
);

We're currently using @sentry/nextjs 6.19.7 and attempted to upgrade to 7.29.0 when we noticed our landing page stopped loading due to what looked like a GraphQL error. We looked through the migration guide to ensure we didn't overlook any required changed, but didn't see anything that applied to our usage. Stepping through the code, we discovered our Next runtime config was empty (resulting in an incorrect GraphQL URL being used and ultimately causing the error we were seeing). This is what lead us to look into what changed with withSentryConfig and looking into next-transpile-modules to find that it does not support passing the next config as a function. If we pin @sentry/nextjs to 7.21.0, the landing page loads without issue.

Fortunately, we realized this particular app longer needs next-transpile-modules so we were able to remove it and have everything work again with @sentry/nextjs 7.29.0. If we needed to keep this plugin around, it appears that swapping the withTM and withSentryConfig calls also works, even though the docs mention withTM should be the outermost plugin. In either case, this was a bit of a surprise to us (and might be for others that are also using next-transpile-modules).

@seanparmelee seanparmelee deleted the withSentryConfig branch January 7, 2023 21:45
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.

2 participants