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(gatsby): IE11 arrow function syntax fix #25642

Closed
wants to merge 1 commit into from

Conversation

hlolli
Copy link

@hlolli hlolli commented Jul 9, 2020

Description

After recent updates, the gatsby project in the organization I work for started to have IE11 syntax errors coming from async-requires.js. After few days of banging our heads against the wall, we learned that babel-plugin-dynamic-import-node transpiles the import statement, but the arrow syntax does not change, and the string append mechanism is happening after the hooks transpiling it are run. This change shouldn't make any functional change in other browser environments, downside being this is bit "uglier" code.

Not supporting IE11 is totally valid argument in 2020, but last I checked the end of life is end of 2020, so let's bear this headache for just few more months ✌️

@hlolli hlolli requested a review from a team as a code owner July 9, 2020 12:08
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 9, 2020
@pieh pieh added topic: webpack/babel Webpack or babel and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jul 9, 2020
@pieh pieh assigned wardpeet and pieh and unassigned wardpeet Jul 9, 2020
@pieh
Copy link
Contributor

pieh commented Jul 9, 2020

Are you sure that this fixes the problem? I was debugging this and what I see is this file being transpiled to is:

exports.components = {
  "component---src-pages-404-js": function componentSrcPages404Js() {
    return Promise.all(/* import() | component---src-pages-404-js */[__webpack_require__.e(1), __webpack_require__.e(0), __webpack_require__.e(4)]).then(__webpack_require__.bind(null, "w2l6"));
  },
  "component---src-pages-index-js": function componentSrcPagesIndexJs() {
    return Promise.all(/* import() | component---src-pages-index-js */[__webpack_require__.e(1), __webpack_require__.e(0), __webpack_require__.e(5)]).then(__webpack_require__.bind(null, "RXBc"));
  },
  "component---src-pages-page-2-js": function componentSrcPagesPage2Js() {
    return Promise.all(/* import() | component---src-pages-page-2-js */[__webpack_require__.e(1), __webpack_require__.e(0), __webpack_require__.e(6)]).then(__webpack_require__.bind(null, "p5nM"));
  },
  "component---src-pages-using-typescript-tsx": function componentSrcPagesUsingTypescriptTsx() {
    return Promise.all(/* import() | component---src-pages-using-typescript-tsx */[__webpack_require__.e(1), __webpack_require__.e(0), __webpack_require__.e(7)]).then(__webpack_require__.bind(null, "+wGG"));
  }
};

(that is using latest version of gatsby).

I think there is more here to understand. This change might fix the symptom and not root cause, but we use arrow functions in other places as well in the runtime, so this change wouldn't be enough to fix this (in addition to all the 3rd party code that could use arrow functions as well)

I think we need more details about this case, before trying to make any changes - can you create issue with more details? Do you perhaps use custom babel setup?

@pieh pieh marked this pull request as draft July 9, 2020 21:30
@hlolli
Copy link
Author

hlolli commented Jul 9, 2020

Defenitely fixes it in our case, we have some babel configuration, let me paste the relevant parts.

.babelrc

{
  "presets": [
    [
      "babel-preset-gatsby",
      {
        "targets": {
          "browsers": ["ie 11"]
        }
      }
    ]
  ]
}

Here we have stuff happening inside the webpack config hook in gatsby-node.js

  config.module.rules = [
    // Omit the default rule where test === '\.jsx?$'
    ...config.module.rules.filter(
      rule => String(rule.test) !== String(/\.jsx?$/)
    ),
    // Recreate it with custom exclude filter
    {
      ...loaders.js(),
      test: /\.jsx?$/,
      // Exclude all node_modules from transpilation
      exclude: /node_modules/,
    },
    {
      test: /node_modules\/(query-string|split-on-first|@sentry)/,
      use: {
        loader: 'babel-loader',
        options: {
          presets: [
            [
              '@babel/preset-env',
              {
                useBuiltIns: 'entry',
                corejs: 2,
                targets: {
                  browsers: 'IE 11',
                },
              },
            ],
          ],
        },
      },
    }

Also browserlist in pacakage.json, some colleauges of mine exhausted themselves in "browserlist" experiments in attempt to solve this. I may be missing some context to why some of these configs were added there.

  "browserslist": [
    "defaults",
    "last 2 versions or > .5% in DE",
    "not IE 11",
    "not IE_Mob 11",
    "not dead"
  ],
``

@hlolli
Copy link
Author

hlolli commented Jul 9, 2020

If all else fails, I was learning about gatsby-dev cli tool to test changes made on gatsby. I never tried the latest and unreleased gatsby without the changes I made, there's to say, this could be automatically resolved by using newer gatsby, we got dependabot update last monday for "gatsby": "^2.23.18",. Let me try to see if this is not a problem in newer gatsby without any modifications.

@pieh
Copy link
Contributor

pieh commented Jul 10, 2020

I think this will not be fixed by latest gatsby versi0on given your custom webpack setup. Can you try adjusting one of your rules to

-test: /node_modules\/(query-string|split-on-first|@sentry)/,
+test: /node_modules\/(query-string|split-on-first|@sentry|\$virtual)/,

In general messing with internal webpack config for js rules like that is bound to cause issues.

In any case - we won't be merging this PR - this fixes the symptom and not a main problem. I think that I did introduce regression in #25057 where the async-requires now lands in node_modules rules (which would explain why you only needed to adjust runtime in this particular file to make it work).

Probably the proper fix here is to adjust virtual modules to not land in node_modules and the more closely match previous setup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: webpack/babel Webpack or babel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants