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

Apply Babel transpilation #41

Closed
Turbo87 opened this issue Jun 27, 2018 · 23 comments
Closed

Apply Babel transpilation #41

Turbo87 opened this issue Jun 27, 2018 · 23 comments

Comments

@Turbo87
Copy link
Contributor

Turbo87 commented Jun 27, 2018

It appears that currently the config/targets.js file is not considered and the code is bundled as is. It would probably be good if the resulting trees would be passed through Babel before being included in the final build.

@bummzack
Copy link

I'd also like this feature. Or an example of how to set the webpack options to enable transpilation (with the same settings as the main app)

@ef4
Copy link
Collaborator

ef4 commented Jun 27, 2018

Transpiling everything always would be safe, but it would potentially slow down builds a lot, and I strongly suspect that for most packages it would be a waste.

It would be nice if there was more agreement about what kind of code to put into NPM packages, or at least some metadata in package.json that would give us hints about it. I think the most popular choice is still to transpile before publishing. But clearly some are going to start wanting to ship ES latest features.

app.import doesn't apply babel either, which means most of the stuff people have always been accessing that way already works without babel.

I think we can implement this as a list of packages that you're opting in to transpiling (with the main app's settings). Each addon can add to the list.

@Turbo87 did you find an example of a package you tried to auto-import that needs babel in order to work in your target browser set?

@bummzack
Copy link

I'd like to use https://www.npmjs.com/package/pdfjs, but it doesn't work in IE 11, because it's not being transpiled.
Although, the demo page doesn't seem to work in IE 11 either.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Jun 28, 2018

and I strongly suspect that for most packages it would be a waste.

I tend to disagree. Even using let/const is a problem and that is done quite a lot in Node.js packages these days.

or at least some metadata in package.json that would give us hints about it

we could look at the engines field, but it probably won't help us that much either

app.import doesn't apply babel either, which means most of the stuff people have always been accessing that way already works without babel.

true, but with app.import it was your responsibility to import the correct precompiled asset, with auto-import that responsibility has shifted

I think we can implement this as a list of packages that you're opting in to transpiling (with the main app's settings). Each addon can add to the list.

I like that idea, although I'd go with an opt-out approach. Most people likely don't test with IE11 and will only find out when they push to prod that their app is no longer working.

I think correctness first, before speed, is the right way to go here, with an option for skipping transpilation if you know that the dep is bundled in a compatible way, or if you only support evergreen browsers.

@Turbo87 did you find an example of a package you tried to auto-import that needs babel in order to work in your target browser set?

I haven't done the full analysis yet (IE11 debugging is great...), but I suspect what broke it for us is https://github.com/sindresorhus/p-retry/ due to the ES6 class being used.

@ef4
Copy link
Collaborator

ef4 commented Jun 29, 2018

Hmm, perhaps you are right. It sounds like we will need to be somewhat flexible in the config for this. I can certainly make both opt-in and opt-out work.

@Duder-onomy
Copy link

Duder-onomy commented Jul 30, 2018

We are running into issues with this right now as well.
https://github.com/sindresorhus/p-map-series uses const, and const cant hang in Safari 9...

Is there a workaround right now? Maybe a branch we can point at? Maybe a way to use this customizing behavior webpack config to force some babel-ing?

@ef4
Copy link
Collaborator

ef4 commented Aug 27, 2018

As a workaround, it should be possible to apply babel via the webpack config. Something like

module: {
  rules: [
    { test: /\.js$/, loader: "babel-loader" }
  ]
}

You may want to experiment with a pattern for test that only covers as much as you actually want to transpile.

@cafreeman
Copy link

I'm curious to see if there's an easy way to share the babel config from the rest of the consuming Ember app with babel-loader. By default, I don't think it picks up any config since there isn't a .babelrc or anything in Ember apps, which means you're likely not getting preset-env or any of the targets set in the config/targets file.

@ef4
Copy link
Collaborator

ef4 commented Sep 17, 2018

It's not too hard and ember-auto-import already does it, see https://github.com/ef4/ember-auto-import/blob/ec307965ebbce248b27fb611ee0cd309056452af/ts/package.ts#L44

@cafreeman
Copy link

cafreeman commented Sep 17, 2018

I was trying something similar from an app's ember-cli-build.js file by letting the app object get fully created and then adding the autoImport property to app.options after the fact, so I could pass in app.options.babel to the loader, like this:

const EmberApp = require('ember-cli/lib/broccoli/ember-app');

module.exports = function(defaults) {
  let app = new EmberApp(defaults, {
    // Add options here
    babel: {
      plugins: [
        require('ember-auto-import/babel-plugin')
      ]
    }
  });

  app.options.autoImport = {
    webpack: {
      module: {
        rules: [
          {
            test: /\.js$/,
            exclude: /(node_modules|bower_components)/,
            use: {
              loader: 'babel-loader',
              options: app.options.babel
            }
          }
        ]
      }
    }
  }

  return app.toTree();
};

Assuming the the EmberApp constructor will run its own version of buildBabelOptions, which we can then pass back in to babel-loader?

@vst
Copy link

vst commented Oct 11, 2018

In my case, there is this module which breaks IE11:

$ yarn why buffer
yarn why v1.10.1
[1/4] 🤔  Why do we have the module "buffer"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "buffer@4.9.1"
info Reasons this module exists
   - "ember-auto-import#webpack#node-libs-browser" depends on it
   - Hoisted from "ember-auto-import#webpack#node-libs-browser#buffer"
info Disk size without dependencies: "232MB"
info Disk size with unique dependencies: "312MB"
info Disk size with transitive dependencies: "312MB"
info Number of shared dependencies: 3
✨  Done in 1.88s.

Will update here if I find why this module gets in my way and nobody else's...

@vst
Copy link

vst commented Oct 11, 2018

My problem was due to ember-cli-plotly which was pulling in both buffer and object-assign dependencies as is and breaking IE11. It is using ember-auto-import, too. I was thinking that the problem was due to my way of using ember-auto-import.

Back to normal after removing it. I don't know how I should feel.

@Kerrick
Copy link

Kerrick commented Nov 13, 2018

As a workaround, it should be possible to apply babel via the webpack config. Something like

module: {
  rules: [
    { test: /\.js$/, loader: "babel-loader" }
  ]
}

Does this change if it's an addon using ember-auto-import? My addon uses ember-auto-import, but after adding this to my addon's index.js file my consuming application doesn't build. I get the following error:

ERROR in Entry module not found: Error: Can't resolve 'babel-loader' in 'C:\Users\kerrick\SecondStreet\front-end\consumer-entry-showcase'

ERROR in Entry module not found: Error: Can't resolve 'babel-loader' in 'C:\Users\kerrick\SecondStreet\front-end\consumer-entry-showcase'Build Error (Bundler)

webpack returned errors to ember-auto-import

@ef4
Copy link
Collaborator

ef4 commented Nov 13, 2018

If the loader is a dependency of your addon, you'll need to resolve it from your addon. One way is:

// added to the webpack config
resolveLoader: {
  alias: {
    'babel-loader': require.resolve('babel-loader')
  }
}

But PLEASE DON'T DO THIS. It's extremely disruptive behavior if adding your addon to a project causes every piece of third party javascript in the entire app to suddenly start going through babel. And this leaves no way for anybody else (either the app or another addon) to change anything about the babel config.

Instead, you need to be careful to not mess with other people's things:

  1. Alias the loader to your own name, so that it won't conflict with anybody else who is also using babel-loader.
  2. Write your rule so it is super-precise. Webpack is extremely bad at this -- almost every example out there does it wrong and matches too much.

Here is one attempt, but test it:

// this is the name of the NPM module you actually want to transpile
let libraryToTranspile = "some-package";
...
    webpack: {
      module: {
        rules: [
          {
            test: new RegExp('^' + path.dirname(require.resolve(libraryToTranspile + '/package.json'))),
            use: {
              loader: 'my-addons-babel-loader',
            }
          }
        ]
      },
      resolveLoader: {
        alias: {
          'my-addons-babel-loader': require.resolve('babel-loader')
        }
      }
    }

@kevincolten
Copy link

kevincolten commented Dec 17, 2018

This seems to run all dependencies through Babel. No guarantees on preventing extraneous transpiling or clobbering others' babel-loader

const { lstatSync, readdirSync } = require('fs');
const { join } = require('path');

//...

autoImport: {
  webpack: {
    module: {
      rules: [
        {
          test: /\.m?js$/,
          use: {
            loader: require.resolve('babel-loader'),
            options: {
              presets: [['@babel/preset-env', { targets: require('./config/targets') }]],
            },
          },
          include: readdirSync('node_modules')
            .map((name) => join(__dirname, 'node_modules', name))
            .filter((source) => lstatSync(source).isDirectory()),
        },
      ],
    },
  },
},

@ef4
Copy link
Collaborator

ef4 commented Jan 25, 2019

This is still definitely a feature that is important.

My goal here is not to allow arbitrary babel configs, but to allow the appropriate subset of the app's babel config to apply also to imported dependencies. It's a "subset" because we may need to disable things like module transpilation that we want to leave up to webpack.

The challenge here is that doing anything other than all-or-nothing isn't simple. You can't easily pick a single dependency and choose to transpile only it, because later if it refactors itself into several sub-dependencies, you would need to make sure those deeper dependencies also get transpiled. So really, choosing to transpile some package implies transpiling it and all its dependencies recursively. And when you do that, you realize you can get conflicting settings for deeper dependencies that are consumed via multiple paths through the dependency graph.

I'm hesitant to just always transpile everything because I'm confident we will find packages that break when you transpile them. I personally hit two babel bugs in the last month. We need to give people a way to at least work around these situations.

I see two alternatives.

Alternative 1 would need:

  • both include and exclude statements
  • each statement would need to be able to target a particular place in the dependency graph (similar to how yarn resolutions lets you talk about a dependency's dependency)
  • each statement would need to affect that package and that package's entire subgraph of dependencies.
  • a rule such that more specific statements take priority over less specific statements

Alternative 2 would start from the assumption that we transpile everything, and only have exclude. This is simpler but do we think there is a high performance cost? Today the vast majority of apps using ember-auto -import aren't transpiling their deps, and they seem to work fine? Or maybe they are broken on certain browsers but don't notice?

Lastly, babel-loader doesn't make it very easy to be selective on a per-package basis (it's entirely file oriented), but I already have code in embroider that solves that problem, so I'm not worried about that aspect.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Jan 26, 2019

Or maybe they are broken on certain browsers but don't notice?

I'm pretty sure that this is the case. Since we use latest Chrome for testing by default those transpilation issues won't show up unless you setup something with Browserstack and explicitly test in IE11.

@ghost
Copy link

ghost commented Feb 5, 2019

I can confirm IE11 breaks after I introduced docx into our application. I have targets.js set up correctly, and simply import * from 'docx'. That library targets ES6 (See their issue)

@allthesignals
Copy link

@kevincolten's solution seems to have worked for me, thank you for that!

Others will need to yarn add babel-loader --dev for it to work. I don't understand babel enough to really understand what I'm doing (why do I need a separate package to do this?). Can someone confirm that's correct?

@allthesignals
Copy link

Yeah, I'm seeing @kevincolten clobber something, but I'm not sure why: Uncaught ReferenceError: _typeof is not defined.

Perhaps it's transpiling something wrong. @ef4 is right, this shouldn't be transpiling everything - but is there a way to easily identify what packages aren't being transpiled? I can't comb through the vendor.js output to find things like arrow functions because... there are dozens of addons and packages that for some reason include comments that have =>.

@ef4
Copy link
Collaborator

ef4 commented May 24, 2019

I'm ready to settle on "Alternative 2" from my comment above. We should implement that.

If somebody wants to implement, it's easier to see how it would work by looking at embroider (because embroider already has a shouldTranspileFile that can efficiently tell which package owns which file, see https://github.com/embroider-build/embroider/blob/2e3ea941da79bde304505e18e49080ceb2d292cb/packages/webpack/src/ember-webpack.ts#L286

We can use the PackageCache out of @embroider/core directly inside ember-auto-import to get ownerOfFile() and implement our own shouldTranspileFile that respects a new exclude option. We'll want to properly roll up the exclude options from the app and all active addons that use ember-auto-import.

@ef4
Copy link
Collaborator

ef4 commented May 24, 2019

Equivalent issue in embroider is embroider-build/embroider#3

We want the same behavior in both.

@ef4
Copy link
Collaborator

ef4 commented Jun 28, 2019

Fixed in #225

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

No branches or pull requests

9 participants