Skip to content
This repository was archived by the owner on Jan 20, 2019. It is now read-only.

Make addons honour app's transpilation by default #89

Closed
cibernox wants to merge 10 commits into from
Closed

Make addons honour app's transpilation by default #89

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e896286
Merge pull request #50 from GavinJoyce/gj/production-code-stripping
nathanhammond Dec 2, 2016
23509d5
Control transpilation
cibernox Dec 7, 2016
cb5a605
Rething RFC
cibernox Dec 7, 2016
34af168
More updates
cibernox Dec 7, 2016
cb4bc9b
Last changes
cibernox Dec 7, 2016
ee22727
Add date
cibernox Dec 7, 2016
77e2f3f
Fix many typos
cibernox Dec 7, 2016
d42c79a
A lot of changes to make clear the focus of the RFC
cibernox Dec 12, 2016
891f7ec
Update 0000-control-transpilation.md
cibernox Dec 12, 2016
74a606c
Update 0000-control-transpilation.md
cibernox Dec 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions active/0000-control-transpilation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
- Start Date: 2016-12-07
- RFC PR: (leave this empty)
- Ember CLI Issue: (leave this empty)

# Summary

Change how addon code is transpiled so it honors user preferences by default, but still
allow addons to be in control of the transpilation if they need to.

# Motivation

Ember CLI has allowed users to write ES6 code and transpile it to ES5/3 since almost the beginning
of the project. It's one of those things that people take for granted.

Today that is done through `ember-cli-babel`, an addon that is in the default blueprint and
that the huge majority of apps use.
Copy link
Contributor

@nathanhammond nathanhammond Dec 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apps and addons


This transpilation process has some sensible defaults that work for most people, but also allow
some user configuration to, by example, disable some transformations enabled by default or enable
some experimental ones.

What is less known is that this configuration only affects the application code (the one in your app
and the code in the `/app` folder of addons).
The transpilation of code living in the `/addon` folder of addons is done according to the
configuration of those addons, which is the default config of ember-cli-babel unless the addons specifies
otherwise.

The reasoning behind that decission is that addons might have special needs so they have to be in
charge their own transpilation. Some examples of addons that do that are [Ember-data](https://github.com/emberjs/data/blob/master/index.js#L115-L125),
[smoke-and-mirrors](https://github.com/runspired/smoke-and-mirrors/blob/master/index.js#L27-L42).

However, an undesired side effect of this approach is that, as it stands today, it means that
the configuration of the user is ignored by addons. That means addons will still transpile features
(p.e. generator functions) even if the users have disabled them in their apps because they targets
only evergreen browsers.

With ES6 being fully implemented in all modern browsers and many ES2016/ES2017 already starting to
Copy link
Contributor

@stefanpenner stefanpenner Dec 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, for this reason. This is quite important to land.

be appear, seems clear that the transpilation level of apps is a moving target and addons should
obey user configuration by default, as opposed of the current approach of transpiling anything that
is not ES5 despite of what the user wants.

This RFC wants to change this behaviour so addons by default honor the host app's preferences,
while allowing those addons to run their own transformations.

# Detailed design

The idea is to still have addons control their own transpilation process, but
make them honor the user's preferences by default.

The implementation is relatively straigtforward. Every addon receives a deep clone of the application's
configuration (available by example on `this.config.babel`), and `ember-cli-babel` must transpile
the addon using those options, which happen to be the options used in the container app.

Note that addons are still in control of the transpilation, they just happen to reuse the app's config
unless the addon author decides otherwise.

Addons that require custom plugins can still get the configuration and modify it, but since it is
a deep clone of the app's configuration, they can be sure that any change they make will be scoped
to that addon and will not affect other addons or the container app.

Note that addon authors _could_ enable optional plugins, like per example [**decorators**](https://github.com/martndemus/ember-font-awesome/blob/master/index.js#L13-L20).
However, since experimental features might become part of the ecmascript spec, this should be
generally discouraged.

# How We Teach This

This has no impact for apps, just for addons.

Most addons will not require any change. Only those that want to modify this the transpilation
Copy link
Member

@rwjblue rwjblue Dec 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not actually see how ember-computed-decorators needs to be changed at all to support this. If you read through the recommended snippet here:

  init: function(app) {
    this._super.init && this._super.init.apply(this, arguments);

    this.options = this.options || {};
    this.options.babel = this.options.babel || {};
    this.options.babel.optional = this.options.babel.optional || [];

    if (this.options.babel.optional.indexOf('es7.decorators') === -1) {
      this.options.babel.optional.push('es7.decorators');
    }
  }

You will notice that it already honors any this.options.babel settings that are present.

process like [ember-computed-decorators](https://github.com/rwjblue/ember-computed-decorators/blob/master/ember-cli-build.js)
or ember data will have to be updated.

# Drawbacks

Although this change *does not* require upgrading to BabelJS, it will be something to take into account
by whatever transition happens last.

# Alternatives

None that I can think of, beside of not doing this.

# Unresolved questions
Copy link
Member

@Turbo87 Turbo87 Dec 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we would also have to figure out how Babel resolves plugins. normally they are referenced by name and loaded relative to the Babel package itself, which can be a problem when e.g. ember-cli-babel is npm linked.

Copy link
Contributor Author

@cibernox cibernox Dec 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently plugins can also be just required and passed to Babel.

Copy link
Member

@Turbo87 Turbo87 Dec 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is somewhat correct, yes. this does not work inside of a .babelrc file though which is JSON-based.

Copy link
Contributor Author

@cibernox cibernox Dec 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that the app should parse its .babelrc file and convert it to a JS object. This object is the one that gets deepcloned and passed to addons so they can manipulate it.

I think that for addons that need to mangle the transpilation the .babelrc manifest is not expresive enough.

Copy link
Member

@rwjblue rwjblue Dec 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note: this RFC does not suggest migration to .babelrc

Copy link
Member

@Turbo87 Turbo87 Dec 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue I know, but I'd like us to be aware of that option. We shouldn't build anything that blocks our way of using those files if we want to.

Copy link
Member

@rwjblue rwjblue Dec 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Turbo87 - Ya, I completely agree. I wasn't very clear either, I think the RFC should address (or at least mention) .babelrc and other concepts that we are required to chart through on our road to babel@6 (see other comment in #89 (comment)).

Copy link

@hzoo hzoo Dec 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as reference there is a PR to add a .js to babelrc babel/babel#4892

create-react-app and others do a require.resolve: https://github.com/facebookincubator/create-react-app/blob/ab3913cc113b2392fa0329f699571bae3c213308/packages/babel-preset-react-app/index.js#L15


For this approach it doesn't matter if the version of babel is 5 or 6, however the plugins and their
names are funamentally incompatible.

- Should addons that depend on `ember-cli-babel` be compiled with their version or with the app's version?
Copy link
Contributor

@nathanhammond nathanhammond Dec 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer we only allow this behavior with homogenous versions. If you're not all Babel6 you get current behavior.

Copy link
Contributor

@stefanpenner stefanpenner Dec 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure.

Allowing the system to progressively transition has its benefits. For example, as add-ons upgrade more and more of the users code-gen would no longer need to cater to the lowest common denominator (ES3).

The downside to this, is you may have some ES3 safe code, mixed with your ES6 safe code. Which seems ok.

This would allow our users to see incremental progress. Especially if the most popular / largest add-ons (like ember-data or even ember) made the change more quickly.

The alternative, requiring homogenous setup. Requires the entire universe to transition before progress is seen.

I suspect coupling incremental + a Quest issue, would enable the best outcomes. (e.g. incremental rewards for our community)

- If the version of babel in the addon wins but the host apps uses babel6, the config cannot be shared. In
that case addons should get the default config?