Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
module.exports = {
root: true,
parser: 'babel-eslint',
parserOptions: {
ecmaFeatures: {
jsx: true,
Expand Down
32 changes: 32 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,38 @@ export default class TodoItem extends React.Component {
}
```

## Object Rest Spread and Class Properties

[Object Rest Spread]() and [Class Properties](https://babeljs.io/docs/en/babel-plugin-proposal-class-properties) are included in `create-react-app` and used in many React projects. To enable these features you must add respective transforms to Babel plugins setting in your `ember-cli-build.js`.

For Babel 6,

1. Add Babel plugin packages to your project `babel-plugin-transform-object-rest-spread` and `babel-plugin-transform-class-properties`
2. In `ember-cli-build.js` file, configure
```js
let app = new EmberApp(defaults, {
babel: {
plugins: ['transform-class-properties', 'transform-object-rest-spread'],
},
});
```

For Babel 7,

1. Install the transforms `npm install --save-dev @babel/plugin-proposal-object-rest-spread @babel/plugin-proposal-class-properties`
2. Add these plugins to your `ember-cli-build.js` file

```js
let app = new EmberAddon(defaults, {
babel: {
plugins: [
'@babel/plugin-proposal-object-rest-spread',
'@babel/plugin-proposal-class-properties',
],
},
});
```

## What's Missing

There is no React `link-to` equivalent for linking to Ember routes inside of
Expand Down
3 changes: 3 additions & 0 deletions ember-cli-build.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ module.exports = function(defaults) {
'ember-cli-babel': {
includePolyfill: true,
},
babel: {
plugins: ['transform-class-properties', 'transform-object-rest-spread'],
},
});

/*
Expand Down
44 changes: 38 additions & 6 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,49 @@
/* eslint-env node */
'use strict';

const react = require('broccoli-react');
const configureJsxTransform = require('./lib/configure-jsx-transform');
const mergeTrees = require('broccoli-merge-trees');
const Funnel = require('broccoli-funnel');
const path = require('path');

module.exports = {
name: 'ember-cli-react',

preprocessTree: function(type, tree) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (type === 'js') {
tree = react(tree, { transform: { es6module: true } });
}
included(parent) {
this._super.included.apply(this, arguments);

return tree;
configureJsxTransform(parent);

parent.registry.add('js', {
name: 'ember-cli-react',
ext: 'jsx',
toTree(tree) {
// get all JSX files and rename them to js
let jsx = new Funnel(tree, {
include: ['**/*.jsx'],
getDestinationPath(relativePath) {
let f = path.parse(relativePath);
if (f.ext === '.jsx') {
return path.join(f.dir, `${f.name}.js`);
} else {
return relativePath;
}
},
});

// apply preprocessing from other babel plugins to the jsx files
let processed = parent.registry
.load('js')
.filter(p => p.name !== 'ember-cli-react')
.reduce((tree, plugin) => plugin.toTree(tree), jsx);

let withoutJsx = new Funnel(tree, {
exclude: ['**/*.jsx'],
});

return mergeTrees([withoutJsx, processed]);
},
});
},

options: {
Expand Down
43 changes: 43 additions & 0 deletions lib/configure-jsx-transform.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
const VersionChecker = require('ember-cli-version-checker');

function requireTransform(transformName) {
return require.resolve(transformName);
}

function hasPlugin(plugins, name) {
for (const maybePlugin of plugins) {
const plugin = Array.isArray(maybePlugin) ? maybePlugin[0] : maybePlugin;
const pluginName = typeof plugin === 'string' ? plugin : plugin.name;

if (pluginName === name) {
return true;
}
}

return false;
}

module.exports = function configureJsxTransform(parent) {
const options = (parent.options = parent.options || {});

const checker = new VersionChecker(parent).for('ember-cli-babel', 'npm');
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

@taras taras Nov 5, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


options.babel = options.babel || {};
options.babel.plugins = options.babel.plugins || [];

if (checker.satisfies('^6.0.0-beta.1')) {
if (!hasPlugin(options.babel.plugins, 'babel-plugin-transform-react-jsx')) {
options.babel.plugins.push(
requireTransform('babel-plugin-transform-react-jsx')
);
}
} else if (checker.gte('7.0.0')) {
if (
!hasPlugin(options.babel.plugins, '@babel/plugin-transform-react-jsx')
) {
options.babel.plugins.push(
requireTransform('@babel/plugin-transform-react-jsx')
);
}
}
};
11 changes: 9 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
"author": "alex@altschool.com",
"license": "MIT",
"devDependencies": {
"babel-eslint": "10.0.1",
"broccoli-asset-rev": "^2.4.5",
"ember-ajax": "^3.0.0",
"ember-cli": "~2.15.1",
Expand All @@ -59,10 +60,16 @@
"husky": "^0.14.3",
"lint-staged": "^5.0.0",
"loader.js": "^4.2.3",
"prettier": "^1.9.2"
"prettier": "^1.9.2",
"babel-plugin-transform-class-properties": "6.24.1",
"babel-plugin-transform-object-rest-spread": "6.26.0",
"broccoli-funnel": "2.0.1",
"broccoli-merge-trees": "3.0.1",
"ember-cli-version-checker": "^2.1.2"
},
"dependencies": {
"broccoli-react": "^0.8.0",
"@babel/plugin-transform-react-jsx": "^7.0.0",
"babel-plugin-transform-react-jsx": "^6.24.1",
"ember-auto-import": "^1.0.1",
"ember-cli-babel": "^6.3.0",
"react": "^15.5.4 || ^16.0.0",
Expand Down
11 changes: 11 additions & 0 deletions tests/dummy/app/components/babel-features.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import React from 'react';

class State {
message = 'supports class properties';
}

export default function BabelFeatures() {
let attrs = { 'data-test': 'allows-object-spread' };
let s = new State();
return <div {...attrs}>{s.message}</div>;
}
7 changes: 7 additions & 0 deletions tests/integration/components/react-component-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ describeComponent(
expect(this.$()).to.have.length(1);
});

it('uses babel features', function() {
this.render(hbs`{{react-component "babel-features"}}`);
expect(this.$('[data-test="allows-object-spread"]').text()).to.equal(
'supports class properties'
);
});

it.skip('throws error when no component found', function() {
expect(() => {
this.render(hbs`{{react-component "missing-component"}}`);
Expand Down