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): Add transpileClientSDK option #5472

Merged
merged 5 commits into from Jul 27, 2022

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Jul 27, 2022

As of version 7.0, our SDK publishes code which isn't natively compatible with older browsers (ones which can't handle ES6 or certain ES6+ features like object spread). If users control the build process of their apps, they can make sure that our SDK code is included in the transpilation they apply to their own code. In nextjs, however, the build process is controlled by the framework, and while users can make modifications to it, they're modifying a webpack config they didn't create, making it a more challenging task.

Fortunately, our SDK can also modify the build process, and that's what this PR does. Nextjs does its transpiling using a loader (either next-babel-loader or next-swc-loader, depending on the nextjs version), controlled by entries in the module.rules section of the webpack config. Normally this transpiling excludes all of node_modules, but we can make it not ignore the SDK by wrapping the exclude function so that it checks first for SDK code (and doesn't exclude it), before applying the normal checks.

Notes:

  • In order to do the wrapping, we first have to find the correct module.rules entries. The default value of module.rules varies by nextjs version, so instead of looking in a known location, we look at all entries. In order to determine which ones to modify, we match on test (a regex for the type of file upon which the rule acts), include (a list of paths upon which the rule will act), and loader (the name of the module actually doing the transpiling). Any rule which would apply next-babel-loader or next-swc-loader to user files gets modified to also apply the loader to SDK code.

  • Because this is only a browser concern, this modification is only made during the client-side build.

  • This only applies to folks trying to support older browsers, and it noticeably increases bundle size, so it's an opt-in process, controlled by a new next.config.js option called transpileClientSDK.

  • While it would theoretically be possible to figure out which builds need this (someone with target: 'es5' in their tsconfig would be a good candidate, for example), the number of locations and ways in which a user can configure that is prohibitively large (a tsconfig with the default name, a tsconfig with a configured name, babel config in webpack config, babel config in package.json, babel config in a file with any one of nine possible names, using a babel preset, using any of a number of different target values, and on and on and on). The option is thus only enabled if a user does so directly.

  • There will be a follow-up docs PR once this option is released in the SDK.

This addresses part of #5452. Non-nextjs users will need to do a similar adjustment on their own, which we will also need to document.

UPDATE: Both documentation tasks are handled by getsentry/sentry-docs#5350.

@vladanpaunovic
Copy link
Contributor

Do we want/need some tests to cover this?

packages/nextjs/src/config/types.ts Outdated Show resolved Hide resolved
options: Record<string, unknown>;
}>;
}>;
rules: Array<WebpackModuleRule>;
Copy link
Member

Choose a reason for hiding this comment

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

Nice! way cleaner than before

@Lms24
Copy link
Member

Lms24 commented Jul 27, 2022

Do we want/need some tests to cover this?

I think this would require us to test the build artifacts of a NextJS app (i.e. to check if our part of the generated JS bundle actually conforms to the ES target level). This is probably quite hard to set up and requires us to add/modify our NextJs testing infrastructure. IMO we can explore this (maybe in our e2e testing initiative) but I'd do this in a separate PR later on.

@Lms24 Lms24 merged commit 22df479 into master Jul 27, 2022
@Lms24 Lms24 deleted the kmclb-nextjs-add-transpile-sdk-option branch July 27, 2022 09:26
Lms24 added a commit that referenced this pull request Jul 27, 2022
Lms24 added a commit that referenced this pull request Jul 27, 2022
const origExclude = rule.exclude;

const newExclude = (filepath: string): boolean => {
if (filepath.includes('@sentry')) {

Choose a reason for hiding this comment

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

I'm totalllllly not good at webpack stuff so this is probably off but: Can there be a case where a rule doesn't have a exclude? In this case should we add exclude to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

In webpack in general, yes, exclude is optional, as you'll see I've made it in the WebpackModuleRule type I added. But in the case of the specific rules nextjs uses for transpilation, it's always there (and has been, in all nextjs versions we support).

Next 10:
Rule using exclude function: https://github.com/vercel/next.js/blob/4f32a03ffa4902a91292d2266ec5f73b8e2b5384/packages/next/build/webpack-config.ts#L981-L999

Next 11:
Rule using exclude function: https://github.com/vercel/next.js/blob/75b7a57e0f0044d9315eb6adbd4231b67799d0b1/packages/next/build/webpack-config.ts#L1121-L1139

Next 12:
Exclude function: https://github.com/vercel/next.js/blob/cee2cf379dfe87ff7a9c9fb88fea4acbfe4a20bc/packages/next/build/webpack-config.ts#L957-L969
Rules where it's used: https://github.com/vercel/next.js/blob/cee2cf379dfe87ff7a9c9fb88fea4acbfe4a20bc/packages/next/build/webpack-config.ts#L1350-L1375

@lobsterkatie
Copy link
Member Author

Do we want/need some tests to cover this?

I think this would require us to test the build artifacts of a NextJS app (i.e. to check if our part of the generated JS bundle actually conforms to the ES target level). This is probably quite hard to set up and requires us to add/modify our NextJs testing infrastructure. IMO we can explore this (maybe in our e2e testing initiative) but I'd do this in a separate PR later on.

Yes to all of the above. See #5195, which I've reopened.

lobsterkatie added a commit to getsentry/sentry-docs that referenced this pull request Jul 28, 2022
…5350)

In getsentry/sentry-javascript#5472, a `transpileClientSDK` option was added to the nextjs SDK to force SDK code to be transpiled along with other app code, primarily for the use of folks wanting to use the SDK in older browsers without support for ES6 or ES6+ features like object spread. This documents that option by:
- Adding a section to the manual setup page (which is where other build-time options live)
- Adding a new 'Build Options' page redirecting people to the relevant section of of the manual setup page, for greater discoverability
- Adding a nextjs-specific include to a new legacy browser support section on the Troubleshooting page. (A generic include has also been added for the non-nextjs JS docs, detailing how to do manually what the added option causes the nextjs SDK to do automatically.)
lobsterkatie added a commit that referenced this pull request Aug 4, 2022
…5516)

In the process of working simultaneously on both #5472 (which adds the `sentry.transpileClientSDK` option to `next.config.js`) and #5473 (which moves the `sentry` options object from `next.config.js` to a new location halfway through the build process, in order to prevent nextjs from throwing warnings), I missed their overlap. As a result, the `transpileClientSDK` option is still currently being retrieved from the old, pre-move location, even though said retrieval happens in the second half of the build, after the move. (Unsurprisingly, it's therefore always undefined, rendering it useless).

This fixes that by pulling it from the new, correct location instead. It also adjusts our types so that future mistakes like this will show up as errors, by creating separate pre-move and post-move `userNextConfig` types and using them as appropriate.

Fixes #5452.
@lobsterkatie lobsterkatie mentioned this pull request Sep 8, 2022
3 tasks
@Noitidart
Copy link

Noitidart commented Feb 14, 2023

Hi there! I upgraded to Next.js 13 and am having similar situation. On the Android 4.4.4 devices (My Raspberry PI and stuff), Sentry isn't starting up, all JS is broken. I think Next.js 13 no longer uses webpack. Does transpileClientSDK work with Next.js 13? Thanks again for the excellent help from last summer!

@Noitidart
Copy link

I think Next.js 13 introduced something called "transpilePackages". Could this be used instead of Sentry's transpileClientSDK to make this work with SWC? https://nextjs.org/docs/advanced-features/compiler#module-transpilation

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.

None yet

4 participants