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

No fetch calls on Edge 16/17/18 or iOS10 after transpiling to esmodules #5452

Closed
5 tasks done
jasonfry opened this issue Jul 22, 2022 · 28 comments · Fixed by #5516
Closed
5 tasks done

No fetch calls on Edge 16/17/18 or iOS10 after transpiling to esmodules #5452

jasonfry opened this issue Jul 22, 2022 · 28 comments · Fixed by #5516

Comments

@jasonfry
Copy link

jasonfry commented Jul 22, 2022

Is there an existing issue for this?

How do you use Sentry?

Self-hosted/on-premise

Which package are you using?

@sentry/react

SDK Version

7.7.0

Framework Version

No response

Link to Sentry event

No response

Steps to Reproduce

  1. When upgrading to v7 and above, Edge 16/17/18 and iOS 10 safari would no longer load our pages.
  2. So I set webpack to include @sentry packages via exclude: /node_modules\/(?!(@sentry)\/).*/, (we normally exclude node_modules). Our @babel/present-env target is targets: { esmodules: true },
  3. After this new build, those browsers started loading our pages okay.
  4. When I view the response to captureMessage it appears to work fine, I get an event_id returned.

Expected Result

I expect the network calls to be made to register these event_id's on all browsers

Actual Result

No network request is made in Edge 16/17/18 or iOS 10 Safari, so these events never get to Sentry.

In more modern browsers I do get the network requests and they work as normal in Sentry.


Internal tracking:

@Noitidart
Copy link

Noitidart commented Jul 22, 2022

Same situation here. On iOS 10 Safari, v7+ is not working. I'm using Next.js. With Sentry v7 installed, all my other JS is broken too. Mine is not self-hosted though, it's using Sentry. A discussion topic is open here on Sentry v7 breaking entire JS system in Next.js - vercel/next.js#38935

@Lms24
Copy link
Member

Lms24 commented Jul 25, 2022

Hi @jasonfry and @Noitidart, thanks for writing in. We switched to ES6 by default with v7 of the JS SDK. So if you want to support browsers that don't support ES6, you need to downcompile the SDK to es5 yourself (e.g. with Babel). An alternative solution is to use our es5 CDN bundles.

Hope this helps!

@Noitidart
Copy link

Thanks @Lms24 can you show us how to do that. As I use Vercel/Next.js because it's batteries included, no messing around with build systems. Sentry integrates with Vercel/Next.js - it used to work fine - but is now breaking their batteries included design.

@jasonfry
Copy link
Author

Hey @Lms24 thanks for the speedy reply!

I thought it should work on iOS10 and Edge 15-18 as they are marked as okay on https://caniuse.com/?search=es6 so therefore should work even without transpiling? I guess my issue as noted above is a step too far for the real question, I only tried transpiling as it wasn't working in these browsers, so @Noitidart's reply is more on-point.

So should these browsers be working with Sentry v7 as they support ES6?

@Lms24
Copy link
Member

Lms24 commented Jul 25, 2022

@Noitidart

This might help you out for Next.JS: https://www.npmjs.com/package/next-transpile-modules

@jasonfry thanks for clarifying. Right, if these browsers support es6 fully, then there shouldn't be a problem with v7. Would be interesting to know what's going on here. Do you have more details (e.g. error messages, stack traces) on the errors you got when using the untranspiled version?

@lobsterkatie
Copy link
Member

Hi, all.

I'm pretty sure I know the culprit here. We use object spread syntax in the SDK, which none of those browsers support.

As far as fixing it goes, a) I tried the package Lukas linked, and it doesn't seem to work, but b) I hacked together something which seems to, and we're going to discuss tomorrow in our standup whether we want to include it in the SDK or just document it as a workaround.

I'll update once we've made a decision.

@Noitidart
Copy link

Noitidart commented Jul 25, 2022

Major <3 guys thanks for helping work this and testing each others ideas! And thanks @jasonfry for getting this conversation started! I was hesitant to start it, not good with words here lol.

It's awesome to hear you tested that package and it didn't work. I was hesitant to use that package too as (next-transpile-modules), it seems the maintenance is a bit uncertain, there's a fork of it, and then many open issues.

@lobsterkatie
Copy link
Member

Update: We discussed it and for nextjs we're going to include my hack in the SDK proper, enabled via a config option (see the linked PR above). For everyone else, we'll document what needs to be done, but (other than in nextjs) we can't change folks' build pipelines, so the fix will have to be on their end.

@Noitidart
Copy link

Absolutely awesome, thank you way way much!! I <3 that decision as it stays true to the Next.js pitch of not having to configure anything, thank you!

Lms24 pushed a commit that referenced this issue 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](https://babeljs.io/docs/en/config-files), 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.
@lobsterkatie
Copy link
Member

lobsterkatie commented Jul 27, 2022

Update: The nextjs transpileClientSDK option is live, in version 7.8.0. Docs are in getsentry/sentry-docs#5350.

@Noitidart
Copy link

Thanks for the update, was eager to try. Will try it out tonight!

@Noitidart
Copy link

Dang, it didn't work for me. Here is my next.config.js:

const { withSentryConfig } = require('@sentry/nextjs');
const fs = require('fs');

const locales = fs
  .readdirSync('./public/locales')
  .map(file => file.replace('.json', ''));

/** @type {import('next').NextConfig} */
const nonSentryNextConfig = {
  reactStrictMode: true,

  i18n: {
    locales,
    defaultLocale: 'en-us'
  },

  sentry: {
    transpileClientSDK: true,
  },
};

// This sets a custom webpack configuration to use your Next.js app with Sentry.
// https://nextjs.org/docs/api-reference/next.config.js/introduction
// https://docs.sentry.io/platforms/javascript/guides/nextjs/
const sentryWebpackPluginOptions = {
  // Additional config options for the Sentry Webpack plugin. Keep in mind that
  // the following options are set automatically, and overriding them is not
  // recommended: release, url, org, project, authToken, configFile,
  // stripPrefix, urlPrefix, include, ignore

  // Suppresses all logs
  silent: true

  // For all available options, see:
  // https://github.com/getsentry/sentry-webpack-plugin#options.
};

// Make sure adding Sentry options is the last code to run before exporting, to
// ensure that your source maps include changes from all other Webpack plugins
module.exports = withSentryConfig(
  nonSentryNextConfig,
  sentryWebpackPluginOptions
);

And my package.json:

image

@lobsterkatie
Copy link
Member

@Noitidart - Can you please try building with and without the new option enabled, and in each case search for ... in .next/static/chunks/pages/_app-<some-hash-here>.js? In my testing, having the option off left me with 178 instances of ... in that file, whereas with the option on, I only had one (which turns out to be inside a string). Do you see the same?

@Noitidart
Copy link

Noitidart commented Jul 28, 2022

I'm using "next": "^12.1.0",

Thanks! I'm getting 186 instances with both on and off. The way I built locally was by running npm run build and I would delete the .next dir before each build as it might have been doing some caches.

Maybe i put the the transpileClientSDK property in wrong place?

npm run build - sentry.transpileClientSDK = false - 186 instances of ...

image

npm run build - sentry.transpileClientSDK = true - still 186 instances of ...

image

@lobsterkatie
Copy link
Member

Huh. That's puzzling.

Are you able to create a tiny repro app? Alternatively, can you please dig into node_modules/@sentry/nextjs/cjs/config/webpack.js and a) confirm that transpileClientSDK is mentioned, and, assuming so, b) stick a debugger in right before the if which mentions it? You'll hit it twice (once for the server build and once for the client build). For the client build (when isServer is false), step through and note:

  • Is userNextConfig.sentry.transpileClientSDK set to true?
  • Is transpilationRules non-empty?
  • Does each entry in transpilationRules have an exclude property, and is it a function?
  • If you stick a debugger inside of if (filepath.includes('@sentry')), do you ever hit it?

In case it's helpful and you don't already have it set up, here's the launch.json config I use in my own test app for debugging next build in VSCode:

launch.json
{
  "version": "0.2.0",
  "configurations": [
    {
      "name": "run `next build`",
      "type": "node",
      "request": "launch",
      "skipFiles": ["<node_internals>/**"],
      "smartStep": false,
      "runtimeArgs": ["--preserve-symlinks", "--trace-warnings"],
      "program": "${workspaceFolder}/node_modules/.bin/next",
      "args": ["build"],
      "outFiles": [
        "${workspaceFolder}/**/*.js",
        "!${workspaceFolder}/node_modules/**/*.js",
        "${workspaceFolder}/node_modules/@sentry/**/*.js",
        "${workspaceFolder}/node_modules/next/**/*.js"
      ],
      "resolveSourceMapLocations": [
        "${workspaceFolder}/**",
        "!**/node_modules/**",
        "${workspaceFolder}/node_modules/next/dist/**",
        "${workspaceFolder}/node_modules/@sentry/*/dist/**"
      ],
      "internalConsoleOptions": "openOnSessionStart"
      // try turning this on if running `next build` in the terminal gives more
      // complete errors than in the debugger
      // "outputCapture": "std"
    }
  ]
}

@Noitidart
Copy link

Wow this is interesting, I'm working on it right now but sharing my progress. Confirmed, node_modules/@sentry/nextjs/cjs/config/webpack.js has transpileClientSDK:
image

Running npm run build then logs my userNextConfig and it doesn't have sentry but has everything else. But here is screenshot with my next.config.js and the code is above here, it definitely has sentry in it - #5452 (comment)

Maybe some caching going on somewhere.

image

@Noitidart
Copy link

Man it's weird. So it's not cached, I added a key called fooFoo and it shows up, but sentry seems to have been removed from it. Any ideas what can be removing it? Checking out the webpack.js code now

image

@Noitidart
Copy link

Noitidart commented Aug 2, 2022

I think I found it, it's with withSentryConfig its deleting sentry, there's two delete calls here, one where userNextConfig is a function, and another when its not.

image

@lobsterkatie
Copy link
Member

Ah, you're right. I get what happened now. I was working on #5472 and #5473 simultaneously, and didn't catch their interaction. Sorry 'bout that. Will push a fix shortly.

@Noitidart
Copy link

Np thanks for the amazing work!

lobsterkatie added a commit that referenced this issue 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.
@jasonfry
Copy link
Author

jasonfry commented Aug 4, 2022

Update: We discussed it and for nextjs we're going to include my hack in the SDK proper, enabled via a config option (see the linked PR above). For everyone else, we'll document what needs to be done, but (other than in nextjs) we can't change folks' build pipelines, so the fix will have to be on their end.

@lobsterkatie Hi! Did the documentation for everyone else get done? I can't find anything apart from the migration guide which seems to be the same as it was. Apologies in advance if it's done and blindingly obvious and I've missed it!

@Lms24
Copy link
Member

Lms24 commented Aug 4, 2022

Hi @jasonfry

The transpileClientSDK option is documented here.

@jasonfry
Copy link
Author

jasonfry commented Aug 4, 2022

The transpileClientSDK option is documented here.

I was looking for the non-Next.js docs, as we're not using Next.js

@Lms24
Copy link
Member

Lms24 commented Aug 4, 2022

Apologies, should have read more carefully.

Documentation for the general Non-NextJS case was added to our docs

Here's the link: https://docs.sentry.io/platforms/javascript/troubleshooting/#supporting-older-browsers

@jasonfry
Copy link
Author

jasonfry commented Aug 4, 2022

Thanks @Lms24 ! We'll give that a go.

@Noitidart
Copy link

Ah, you're right. I get what happened now. I was working on #5472 and #5473 simultaneously, and didn't catch their interaction. Sorry 'bout that. Will push a fix shortly.

@lobsterkatie I had a thought, while you were working on this. Did you want to consider making transpileClientSDK default to true? So then it matches the same range of browsers Next.js says its delivers on. Otherwise guys might try it on older browser and be baffled on why its not working even though Next.js said it was.

@lobsterkatie
Copy link
Member

I can see arguments for and against. Because it has a legit bundle size impact, I think we'll leave it off for now, but depending on what feedback we get going forward, we could think about auto-enabling it in the future.

P.S. With zero hard feelings, I do feel compelled to point out that not all developers are guys. 🙂

@Noitidart
Copy link

That's fair, no problem if they don't was just a thought, thanks for the comment!

Haha of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants