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

Auspice extensions are not being (fully) transpiled, causing ES6 code to be leaked into the bundle. #902

Open
ivan-aksamentov opened this issue Feb 21, 2020 · 5 comments

Comments

@ivan-aksamentov
Copy link
Member

ivan-aksamentov commented Feb 21, 2020

This issue affects both nextstrain.org and auspice.

Description

Recently a dependency to 'iso-639-1' package was introduced in nextstrain.org, required for language picker functionality, implemented as a part of nextstrain's extension to auspice:
nextstrain/nextstrain.org/auspice/client/languageSelector.js#L2

This uncovered a defect in auspice extension system. The iso-639-1 package distribution contains transpiled ES5 bundle and original ES6 modules:
https://github.com/meikidd/iso-639-1/blob/master/package.json#L5-L6

Auspice's webpack somehow picks ES6 code and fails to transpile it, so that ES6 classes (in particular, class keyword), go (minified) straight into the bundle:

    class i {
      static getLanguages(e = []) {
        return e.map(e => ({
          code: e,
          name: i.getName(e),
          nativeName: i.getNativeName(e)
        }));
      }
      static getName(e) {
        return i.validate(e) ? a[e].name : "";
      }
      static getAllNames() {
        return Object.values(a).map(e => e.name);
      }
      static getNativeName(e) {
        return i.validate(e) ? a[e].nativeName : "";
      }
      static getAllNativeNames() {
        return Object.values(a).map(e => e.nativeName);
      }
      static getCode(e) {
        return (
          Object.keys(a).find(n => {
            const t = a[n];
            return (
              t.name.toLowerCase() === e.toLowerCase() ||
              t.nativeName.toLowerCase() === e.toLowerCase()
            );
          }) || ""
        );
      }
      static getAllCodes() {
        return Object.keys(a);
      }
      static validate(e) {
        return a.hasOwnProperty(e);
      }
    }

This portion of code corresponds to class ISO6391:
https://github.com/meikidd/iso-639-1/blob/master/src/index.js#L3

and its ES5 version is shipped here (but ignored by our webpack):
https://github.com/meikidd/iso-639-1/blob/master/build/index.js#L1574

This could be due to:

  • extensions are not being transpiled at all
  • only their dependency packages are not being transpiled
  • both of the above

Now, normally, dependencies are excluded in babel-loader, but one may need to selectively whitelist some of the packages, if package module target does not meet the requirements. I tried to whitelist iso-639-1, in auspice, but that did not work (see below).

Note that in the case of nextstrain.or we know what to whitelist because the auspice and extension come from the same team. I am sure we will find a way to patch it. However, for a third party extensions, auspice can not know (at runtime) which packages are being used by the extension, therefore it can not know what to whitelist.

Basically, current users of the extension system have to ensure that their modules are compatible with their desired target browsers. One way is to transpile the modules as a separate step.

Attempted fix

Adding .browserslistrc

[development]
Last 2 versions

[production]
cover 99%
ie >= 9

and whitelisting iso-639-1 for transpilation in webpack.config.js:

  /* which directories should be parsed by babel and other loaders? */
  const directoriesToTransform = [
    path.join(__dirname, 'src'),
    path.join(__dirname, 'node_modules/iso-639-1'),
  ];

That somehow did not work. class keyword is still in the bundle.

I still only understand the extension system vaguely, and is puzzled right now.

Context

This was uncovered by @emmahodcroft as another "nextstrain.org is broken in Internet Explorer again" issue, but this time it is a bit deeper than usual and also affects at least all the users with browsers that do not support ES6 classes, which are all the browsers prior to ~2016-2017, (~7% of global share, see caniuse.com/es6-class).

Note how Firefox 45 is listed as green on caniuse for ES6 classes, but still fails to render, probably due to other untranspiled code fragments appearing in the bundle later on.

@ivan-aksamentov
Copy link
Member Author

ivan-aksamentov commented Feb 22, 2020

nextstrain/nextstrain.org#119 removes classes, but the old browsers will now crash on const anyways. The breakage is down from 7% to 2-3% https://caniuse.com/const

Obviously, the transpilation issue itself is not solved. I cannot even find why that happens.

These consts now come not from iso-639-1 (it's fully-ES5 now), but from somewhere else - either from a module or from extension's code.

jameshadfield added a commit to nextstrain/nextstrain.org that referenced this issue Feb 26, 2020
This moves the "auspice" server code to `./src` so that all the auspice client code is in it's own directory (./auspice-client`). This helps prevent `auspice build` pulling in code it shouldn't and which it doesn't correctly transpile. Note that this is a work-around for the deeper problem explained at nextstrain/auspice#902.

Moving server code to `./src` makes sense now that the nextstrain.org capabilities have advanced. Some file renaming may be in order here for clarity, and we should consider moving `authn.js` and `redirects.js` into `./src`.
@jameshadfield
Copy link
Member

jameshadfield commented Mar 4, 2020

Adding this to https://github.com/orgs/nextstrain/projects/5 in case webpack experts see this! Please reach out to myself or @ivan-aksamentov if you need a better description of nextstrain / auspice extensions

@obahareth
Copy link

Hey everyone, I actually built are-you-es5 specifically for finding out which node_modules aren't ES5 and getting a regex to include them in babel-loader's pipeline and have them be transpiled to ES5.

@hydrosquall opened an issue (reference to it is right above this comment) to see if are-you-es5 could help with this and I'm happy to help out however I can.

@salvatore-fxpig
Copy link
Contributor

The reason why whitelisting attempt does not work here is because the module iso-639-1 is pulled from the nextstrain.org/node_modules dir and not from auspice/node_modules.

So whitelisting it like this:

  const directoriesToTransform = [
    path.join(__dirname, 'src'),
    /node_modules\/iso-639-1/
  ];

Actually works pretty well.

For a more general (but not fully general) solution that could help reducing guesswork for people developing extensions you might try something like:

    resolve: {
      mainFields: ['browser', 'main', 'module'],
      alias: aliasesToResolve
    },

To force default resolution to pretranspiled version for modules that have both (this is the case for iso-639-1). May need some testing though.

@salvatore-fxpig
Copy link
Contributor

Another minor thing I've noticed about this is the following:

Auspice's webpack tries to source react relative to the root folder of each file, which has implications for extensions that contain react.
Namely, if auspice is installed as a package, auspice's react will be hoisted to the main node_modules folder, and so the extension's react and auspice's react will resolve to the same instance of react, and all will be well.

However, if you trying compiling auspice with extensions, and your extensions are in a separate folder, and contain react, webpack will resolve two different reacts and the page will immediately crash on load (react does demand to be resolved to just one single package).

To test it out you can:

  1. Clone the auspice repo locally
  2. Clone the nextstrain.org repo locally.
    Go to auspice's root and do something like
npm run build --  --extend ../nextstrain.org/auspice-client/customisations/config.json

Then serve it and see React fail with mention of "bad hooks" which turns out to be caused by 2 reacts.

An easy patch for this is aliasing react explicitly in auspice's webpack:

const aliasesToResolve = {
    "@extensions": '.', /* must provide a default, else it won't compile */
    "@auspice": path.join(__dirname, 'src'),
    "@libraries": path.join(__dirname, 'node_modules'),
    "react": path.join(__dirname, 'node_modules/react')
  };

But I'm not 100% sure that this hasn't some additional unintended effect.

You might simply wanna add a mention somewhere that for the extensions to work auspice has to be installed as a dependency of the project that tries to extend it and has to be compiled from the root of that project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Front-end nCoV-related work to be done
  
Bigger (harder) issues. Roughly in or...
Development

No branches or pull requests

5 participants