Allows to configure babel options via ember-cli-build

[taras]

Purpose

Object Rest Spread and Class Properties are transforms that are included with create-react-app. As such, they're considered common features in React applications. ember-cli-react doesn't allow these features to be used in JSX which limits capabilities for developers who're used to using these features in React applications.

Approach

This PR removes broccoli-react in favour of using EmberCLI's Babel Preprocessor. This makes it possible to configure JSX by adding transforms to the babel configuration in ember-cli-build.js. Here is an example of this file with several plugins installed.

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

module.exports = function(defaults) {
  let app = new EmberApp(defaults, {
    babel: {
      plugins: [
        'transform-decorators-legacy',
        'transform-object-rest-spread',
        'transform-class-properties'
      ]
    }
  });

  return app.toTree();
};

TODO

• Added information about adding plugins to README
• [ ] Wait for broccoli-react 0.9.0 to be released Please publish 0.9.0 to npm edd/broccoli-react#12
• [ ] Confirm the configuration API

Unresolved

(Not relevant anymore)

This PR adds a new way to configure a Babel compiler with new options. Here is an example of adding 3 plugins to the Ember babel and ember-cli-react's babel.

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

module.exports = function(defaults) {
  let app = new EmberApp(defaults, {
    babel: {
      plugins: [
        'transform-decorators-legacy',
        'transform-object-rest-spread',
        'transform-class-properties'
      ]
    },
    'ember-cli-react': {
      babelOptions: {
        plugins: [
          'transform-decorators-legacy',
          'transform-object-rest-spread',
          'transform-class-properties'
        ]
      },
    },
  });

  return app.toTree();
};

Adding another way to configure Babel is 💩. It would be better to use Ember's Babel compiler but that might require changes to broccoli-react or a different approach to adding JSX to the Ember app. Alternative approach would be to drop broccoli-react in favour of adding JSX plugin to Ember's babel compiler. I'm not sure what impact that would have, so I'd like to open this to discussion.

[pswai]

In my opinion it actually makes sense to separate the babel config here with ember-cli-babel. AFAIK ember-cli-babel imposes some limitation on its configuration and we might not want that. Also, for projects that migrate away from Ember to React, most likely they are still stuck with Babel 5. The config of Babel 6 is not backward compatible so those projects will have issues.

I have thought about dropping broccoli-react and integrate babel in ember-cli-react directly. Would you like to take a stab at it? :)

[taras]

I have thought about dropping broccoli-react and integrate babel in ember-cli-react directly. Would you like to take a stab at it? :)

Depends on how much work it'll be :) but seriously, what challenges do you envision? It seems that ember-cli-react is mostly the resolver and the component. Babel only needs the JSX transform. Is there anything else to it?

[pswai]

I think that's pretty much about it. The JSX transform happens in preprocessTree so I think the work needed here is just about putting broccoli-react's logic there. The optional challenge here is to migrate the deprecated broccoli-filter to broccoli-funnel and it should not be too hard.

Once this is in we have better control of the transform and it allows us to do a lot more things! (e.g. React components in .js extension, etc)

[taras]

@pswai I updated the PR to remove broccoli-react and configure Babel to automatically add JSX parsing. I borrowed configure-jsx-transform.js from @alexlsfroscia https://github.com/alexlafroscia/ember-react-components/tree/master/lib

[taras]

Ping

[pswai]

@taras Sorry I was a little busy last week, let me have a look

[pswai]

This is actually utilising ember-cli-babel from the Ember App, which might be problematic if the app is using older Ember that uses babel 5. I would suggest to hook up babel ourselves so that we control how to compile JSX files ourselves.

[taras]

This was what I had in mind - ie use Babel from the app. Are you thinking that we add ember-cli-babel as a dependency to ember-cli-react and use that?

[pswai]

I am actually thinking to use babel-core directly instead of relying on ember-cli-babel. The advantage of this is that this will work for any app with any version of ember-cli-babel. What do you think?

[taras]

hmm... we could add another condition to lib to check if they're using babel5. Is there any difference between babel5 & babel6?

It seems to come down to wether or not we're using the app's babel. I would prefer to use app's babel so babel features that are available in React and Ember are consistent. It would be pretty annoying to start using a feature in React then realize that it's not available in the Ember app and vice versa.

[pswai]

Babel 6 is incompatible with Babel 5, at least from the configuration. Also Babel 6 removed "require interop" that caused lots of application to have a hard to upgrading last time.

I agree separating Babel config is troublesome but I think there are situations that we want to transpile React files differently with Ember files. Not sure if I am worrying too much :P

@alexgb any input?

[alexgb]

Ok, I think I'm getting it now. If I understand correctly, you're saying that when using broccoli-react the babel configuration options applied to the host app have no affect on JSX files? I see that appears to be because broccoli-react already uses its own babel-core. It also exposes configuration for its babel transformation that we should be able to use to add additional plugins here. Is that a viable path forward here?

I'm trying to avoid adding too much complexity to ember-cli-react and also want to maintain compatibility with earlier versions of Ember CLI that use babel 5.x. But I do see how it's important to have reast spread and class properties support in JSX files, so let's figure out the right solution.

[taras]

If I understand correctly, you're saying that when using broccoli-react the babel configuration options applied to the host app have no affect on JSX files?

Yes

It also exposes configuration for its babel transformation that we should be able to use to add additional plugins here. Is that a viable path forward here?

This is where this PR started. If you look at the start of this PR, the first commit is da25380 which makes it possible to configure broccoli-react.

But I do see how it's important to have rest spread and class properties support in JSX files, so let's figure out the right solution.

I suggested to remove broccoli-react in favour of using using app's babel because when you start using these features in React, you'll probably want to use them in the Ember app. Then you find it doesn't work in the Ember app because your configuration is only applied to ember-cli-react. Then you end up with something like this,

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

const plugins = [
  'transform-decorators-legacy',
  'transform-object-rest-spread',
  'transform-class-properties'
];

module.exports = function(defaults) {
  let app = new EmberApp(defaults, {
    babel: {
      plugins
    },
    'ember-cli-react': {
      babelOptions: {
        plugins
      },
    },
  });

  return app.toTree();
};

If we're ok with this, then I can just revert the changes and that I made in last few commits and push the original PR. Let me know what you think we should do.

[alexgb]

Gotcha. Thanks for walking me through your thinking. So the main decision is between using the host app's babel to process JSX or an isolated babel-core, which feels like a choice between a two evils:

1. If we use the host app's babel, then ember-cli-react has a somewhat brittle binding to host app's babel version to add JSX support, which we must maintain as babel is updated.
2. If we use isolated babel-core then we force the user to know how to configure both babel6, as used by broccoli-react, and their own babel version.

I think I'm coming around to using the host app's babel, but don't feel strongly. If we go this route I think it would be wise for us to add a peer dependency on ember-cli-babel so that we don't imply support for future unreleased versions of babel. Also, how is this currently working on babel 5? The tests seem to be passing on versions of ember-cli older than 2.12, which should be using babel 5, but your code only seems to inject JSX support on babel 6 or babel 7.

[taras]

If we go this route I think it would be wise for us to add a peer dependency on ember-cli-babel so that we don't imply support for future unreleased versions of babel.

👍

Also, how is this currently working on babel 5? The tests seem to be passing on versions of ember-cli older than 2.12, which should be using babel 5, but your code only seems to inject JSX support on babel 6 or babel 7.

Are you sure it's using babel 5? The fact that it doesn't change babel dependency suggests to me that it's using whatever is installed before ember-try runs. I could be wrong though. We might be able to add a test for babel 5.

[alexgb]

Yep, I think you're right, which concerns me because we do test and indicate support for Ember versions down to 1.13 and it's unfortunate that ember-try just exercises ember and ember-data versions, but not the full set of ember-cli dependencies for a particular release.

So yes, would be good to test babel5 compatibility, but not sure how to test that end to end.

[pswai]

Actually we should be able to continue using preprocessTree here. Instead of using broccoli-react we do babel transpile and feed into funnel here directly.

[taras]

No worries, just want to make sure I keep the PR alive :)