ember-cli / rfcs

Archive of RFCs for changes to ember-cli (for current RFC repo see https://github.com/emberjs/rfcs)
45 stars 54 forks source link

Problems with standardized targets RFC #95 and javascript #104

Closed kanongil closed 6 years ago

kanongil commented 7 years ago

While RFC #95 allows efficient transpilation and targeting modern JS engines, flaws appear when actually applying this.

I have 2 concrete issues, which both occurs when targeting modern browsers that allow ES6 features.

The first major issue is minification: The currently used UglifyJS does not support ES6 syntax and throws arcane errors when trying to produce a minified build. This can be somewhat alleviated by using ember-cli-babili, which supports such syntax. However, there are 2 specific reasons why this works: First, it only applies ES5-safe transforms. To do anything else, it would need to take into account the full browser matrix. Secondly, it uses the babel engine, ensuring syntax compatibility. Without this, it will break when new syntax elements are introduced to the language.

The second issue is testing: Currently PhantomJS is used, which again does not support ES6 syntax. This has a quick fix (add a PhantomJS compatible browser to the targets when testing), but this somewhat undermines the actual testing. Even if this was replaced with a more compatible JS engine, there is no guarantee that it would match the targets that the user has specified, and will thus always require a test-specific transpilation target (which again undermines the validity).

With these 2 issues I think that the targets RFC actually ends up doing more harm than good. It is great to leverage the tremendous effort that has been put into creating these browser targets, but not so great when the supporting eco-system has to do a comparable effort to support this, and we have a deep vendor lock-in.

TDLR: The targets needs to respected at 3 tiers of JS, rather than just the one that was intended, which greatly complicates the eco-system.

kanongil commented 7 years ago

Alternative rough proposal idea Create and use compatibility tiers, e.g.: ES5, ES6-limited, ES6, etc.

The app would declare the base tier, e.g. ES6, similar to the target, which the application code would adhere to. An addon would specify which tier the code targets, and when transpiling the code, target the lesser of the two.

This enables fixed compatibility targets for minification and testing. Eg. ember-cli-uglify would target ES5, while ember-cli-babili could target ES6. Once their compatibility improves, this target could likewise increase.

As I see it, this might actually work with the browser targets, where the tier works as a cap.

kanongil commented 7 years ago

Come to think of it, what this all boils down to, is that the targets RFC needs a way to cap it to a known feature set.

cibernox commented 7 years ago

I think those are not really issues with the targets rfc, but I'll comment one by one.

The first major issue is minification [...]

The minification issue I think it's being worked on. The options are babili or using the harmony branch of uglify-js, both of which should work. I think that before 2.13 beta hits stable we should have a solution for this problem in the default blueprint.

The second issue is testing [...]

This one I don't think it's really an issue. Phantomjs is indeed an ES6-incapable browser. If you want to run your tests in Phantomjs I think that it is expected that you must transpile your code to ES5. It is true that there is no such thing as "phantomjs" target in babel-preset-env, so for the time being I think that adding IE9 as a traget will transpile enough to make phantomjs work. If your application is intended to work in es6 capable browsers I'd argue that you should not test in phantomjs either way.

The complain is still valid in the sense that there is a misalignment between making dead simple for people to ship ES6 code if they want but at the same time the default blueprint comes with an ES6 incapable minifier and the default browser in tests is not ES6 aware.

I think that the follow-up actions are:

/cc @rwjblue

kanongil commented 7 years ago

While there is indeed solutions to both issues, they are all short-term solutions that requires adaption to specific use cases.

With my last comment, I actually think that the targets RFC is ok, if it is extended with a mechanism to cap the feature set. Until now, this has been an implicit cap called ES5. With this RFC, it is suddenly unbounded to all of future JS.

Given this unboundedness, any build configuration can fail on any day, if the target browsers implement a feature not supported by the testing / minify toolset.

The solution, as I see it, is to extend the RFC with specific feature caps, which the toolset can be verified against.

rwjblue commented 7 years ago

FYI - Specifying either of the following targets configs will perform a full ES5 transpilation (similar to babel@5):

module.exports = {
  browsers: [ ]
};
module.exports = {
  browsers: [
    uglify: true,
    /* any other values */
  ]
};

In the end, the resulting build is completely within the control of the application developer. If they do not like/want this feature they can use a targets file like the above.

kanongil commented 7 years ago

Ok, this reduces the problem to: any adaptive build configuration can fail on any day.

I still call that a major problem.

rwjblue commented 7 years ago

Yep, I agree that if an application does not control its dependencies properly (e.g. yarn.lock or npm-shrinkwrap.json in some cases), their build could randomly fail on any day with an adaptive target configuration.

This is obviously not a new issue though. If dependencies are being randomly updated/changed without testing, you will always have the potential of build failure at any time (completely unrelated to targets).

kanongil commented 7 years ago

@cibernox made me realise that the uglify: true option actually represents what I want, by capping the feature set to ES5. Essentially, I just need more for e.g. ES6, and to apply the cap from inside a plugin.

cibernox commented 7 years ago

From slack conversation.

The problem with adaptative build took is that all the tooling in the chain has to be "evergreen" and be able to parse the new JS features that your browser matrix supports. In JS today that means babel and in the future perhaps typescript.

The problem is that JS tooling is also becoming "evergreen", so capping at a specific feature-set (say, ES6) is also missleading because you need to know what your tooling is able to support. By example (made up situation), if the harmony branch of uglify supports ES6 but not ES7, you need to know that and update when they support it. And if it supports ES6 but only half of ES7 you need to know what features exactly it doesn't support or settle on a smaller-than-neccessary set of features.

That is why I think that tooling should be treated like any other target and let babel-preset-env know that to transpile (the browsers: ['uglify'] approach that @rwjblue mentioned)

In practice that means that if a new tool in the ecosystem that is not based in babel appears, it should be added to babel-preset-env. It shouldn't be that hard, since it supports aliases. In case the tool named foobar becomes widespread we could make a PR to babel-preset-env / browserlist aliasing foobar to ie9 or other target is a compatible set.

In practice I think that, other than minificiation, this is small chances of that happening in practice.

kanongil commented 7 years ago

Note that with my suggestion, an addon with something like the typescript compiler could know whether to target ES5 or ES2017, and not always use ES5, or require an additional babel transpilation step.

Or do you want the typescript compiler to figure out what to target from the browser targets? I suspect this will greatly complicate the implementation and maintenance work.

cibernox commented 7 years ago

what I would use it to configure typescript to output 2017 or whatever state of the art in JS is and have a second pass with babel to downgrade it

kanongil commented 7 years ago

Another angle: Babel only supports a subset of ES6, and will never support 100%. How do we enable plugin authors to safely use these features?

Say my plugin uses an external lib that utilizes a FancyArray class, subclassing Array. I would need a way to declare that I require an ES6 compatible runtime. This could be in the plugin description, but I would much prefer to declare it in a way that can be managed by the tooling, which could leverage the browser targets configuration to alert me if it is incompatible.

Extending my proposal, it could be split into syntax and runtime targets, where this plugin would have syntax: 'ES6', runtime: 'ES6', while a plugin that merely uses ES6 syntax, would have syntax: 'ES6', runtime: 'ES5'.

Btw, am I right in assuming that plugins shouldn't specify browser targets?

cibernox commented 7 years ago

Part of the RFC was that addons are in control of their own transpilation. The browsers the user wants to target is a directive that "Good Addons"™ should try to follow, but if an addon needs an special polyfill or custom transformation they can still can do whatever they want with their tree.

A good example of addon that conditionally adds a runtime polyfill depending on the user targets is ember-maybe-import-regenerator.

kanongil commented 7 years ago

A good example of addon that conditionally adds a runtime polyfill depending on the user targets is ember-maybe-import-regenerator.

That is a crappy example. It doesn't respect any kind of targets. When included, it will just ensure that the polyfill is there, one way or another.

rwjblue commented 7 years ago

Check again? It asks ember-cli-babel if the polyfill is needed (which is based on targets), and only imports it if it is...

kanongil commented 7 years ago

My take from all of this, is that you want a world where we essentially babel all the things, even if it means that code is transpired, and then run through the babel engine maybe 3 times to do a test.

cibernox commented 7 years ago

@kanongil That is not true.

In this line you can see how the addon queries if a feature is needed before including the polyfill

cibernox commented 7 years ago

@kanongil I think that all the transpilation is made in only one pass and it used to be two passes. Each addon's tree might be transpiled with a different configuration that is unique for that tree, but that is still a single pass in the sense that the same code doesn't go though babel more than once.

kanongil commented 7 years ago

Check again? It asks ember-cli-babel if the polyfill is needed (which is based on targets), and only imports it if it is...

Ah, now I see. While clever, it is still very application specific, and targets that specific feature of the babel engine.

cibernox commented 7 years ago

@kanongil That logic is less tied to babel than it seems. ember-cli-babel offers a convenient function to check if a feature is required by the given build matrix, but actually that logic lives in babel-preset-env which is the state of the art in JS-feature detection, but it can be used even if the app is transpilted with something else (typescript) or not transpiled at all.

I know it sounds weird installing babel and use it for feature detection instead of for transpilation, but right now is the best tool out there.

kanongil commented 7 years ago

@cibernox Interesting. It is still limited to the features that babel support, though.

@rwjblue Regarding the uglify: true option that you suggested, it seems mightily wrong to specify it along with normal browser targets as it is a babel-preset-env specific option. How is eg. a css auto-prefixer supposed to interpret it?

It would probably be safer to pre-parse the browser targets, so e.g. ['last 1 chrome versions'] becomes ['chrome 56'], before handing it over to the addons. That way you are also certain that all addons interpret it the same way, based on the db in the main app.

cibernox commented 7 years ago

@kanongil are you aware of any JS feature that this cannot detect?babel-preset-env uses data from http://caniuse.com and http://kangax.github.io/compat-table/es6, so it should know if feature is available regardless of if that feature is transpilable/polyfillable or not (p.e. ES6 Proxy)

kanongil commented 7 years ago

@cibernox Afaik, it only maps features to supported plugins. The database used is this one: https://github.com/babel/babel-preset-env/blob/master/data/plugins.json