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

Add transform to support arrow function class properties #322

Merged
merged 14 commits into from
Sep 15, 2016
Merged
Show file tree
Hide file tree
Changes from 12 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
4 changes: 2 additions & 2 deletions .babelrc
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
{
"presets": ["es2015", "react"]
}
"presets": ["es2015", "react", "stage-2"]
}
6 changes: 1 addition & 5 deletions .eslintrc
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
{
"extends": "airbnb",
"parserOptions": {
"ecmaFeatures": {
"experimentalObjectRestSpread": true
},
},
"parser": "babel-eslint",
"rules": {
"no-underscore-dangle": ["error", { "allow": ["__REACT_HOT_LOADER__"] }],
"no-console": ["error", { allow: ["error"] }],
Expand Down
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,10 @@
"devDependencies": {
"babel-cli": "^6.7.5",
"babel-core": "^6.7.6",
"babel-eslint": "^6.1.2",
"babel-preset-es2015": "^6.6.0",
"babel-preset-react": "^6.5.0",
"babel-preset-stage-2": "^6.5.0",
"enzyme": "^2.2.0",
"eslint": "^2.9.0",
"eslint-config-airbnb": "^8.0.0",
Expand Down
43 changes: 43 additions & 0 deletions src/babel/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,19 @@ const buildTagger = template(`
})();
`);

const buildNewClassProperty = (t, classPropertyName, newMethodName) => {
const returnExpression = t.callExpression(
t.memberExpression(t.thisExpression(), newMethodName),
[t.spreadElement(t.identifier('params'))]
);

const newArrowFunction = t.arrowFunctionExpression(
[t.restElement(t.identifier('params'))],
returnExpression
);
return t.classProperty(classPropertyName, newArrowFunction);
};

module.exports = function plugin(args) {
// This is a Babel plugin, but the user put it in the Webpack config.
if (this && this.callback) {
Expand Down Expand Up @@ -125,6 +138,36 @@ module.exports = function plugin(args) {
node.body.push(buildSemi());
},
},

Class(classPath) {
const classBody = classPath.get('body');

classBody.get('body').forEach(path => {
if (path.isClassProperty()) {
const { node } = path;

// class property node value is nullable
if (node.value && node.value.type === 'ArrowFunctionExpression') {
const params = node.value.params;
const newIdentifier = t.identifier(`__${node.key.name}__REACT_HOT_LOADER__`);

// arrow function body can either be a block statement or a returned expression
const newMethodBody = node.value.body.type === 'BlockStatement' ?
node.value.body :
t.blockStatement([t.returnStatement(node.value.body)]);

// create a new method on the class that the original class property function
// calls, since the method is able to be replaced by RHL
const newMethod = t.classMethod('method', newIdentifier, params, newMethodBody);
path.insertAfter(newMethod);

// replace the original class property function with a function that calls
// the new class method created above
path.replaceWith(buildNewClassProperty(t, node.key, newIdentifier));
}
}
});
},
},
};
};
4 changes: 4 additions & 0 deletions test/AppContainer/.babelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"presets": ["es2015", "stage-2", "react"],
"plugins": ["../../src/babel"]
}
229 changes: 229 additions & 0 deletions test/AppContainer/AppContainer.dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,235 @@ function runAllTests(useWeakMap) {

expect(wrapper.text()).toBe('new render + old state');
});

it('replaces children class methods', () => {
const spy = createSpy();

class App extends Component {
componentWillMount() {
this.state = 'old';
}

shouldComponentUpdate() {
return false;
}

handleClick() {
spy('foo');
}

render() {
return (
<span onClick={this.handleClick}>old render + {this.state} state</span>
);
}
}
RHL.register(App, 'App', 'test.js');

const wrapper = mount(<AppContainer><App /></AppContainer>);
wrapper.find('span').simulate('click');
expect(spy).toHaveBeenCalledWith('foo');
expect(wrapper.text()).toBe('old render + old state');

spy.reset();
{
class App extends Component {
componentWillMount() {
this.state = 'new';
}

shouldComponentUpdate() {
return false;
}

handleClick() {
spy('bar');
}

render() {
return (
<span onClick={this.handleClick}>new render + {this.state} state</span>
);
}
}
RHL.register(App, 'App', 'test.js');
wrapper.setProps({ children: <App /> });
}

wrapper.find('span').simulate('click');
expect(spy).toHaveBeenCalledWith('bar');
Copy link
Collaborator

Choose a reason for hiding this comment

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

@calesce it might be worth it to test that the method is replaced without remounting the component, eg. like it is done here https://github.com/gaearon/react-hot-loader/blob/next/test/AppContainer/AppContainer.dev.js#L182

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, doesn't hurt.

expect(wrapper.text()).toBe('new render + old state');
});

it('replaces children class property arrow functions', () => {
const spy = createSpy();

class App extends Component {
componentWillMount() {
this.state = 'old';
}

shouldComponentUpdate() {
return false;
}

handleClick = () => {
spy('foo');
};

render() {
return (
<span onClick={this.handleClick}>old render + {this.state} state</span>
);
}
}
RHL.register(App, 'App', 'test.js');

const wrapper = mount(<AppContainer><App /></AppContainer>);
wrapper.find('span').simulate('click');
expect(spy).toHaveBeenCalledWith('foo');
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above about testing that it doesnt remount

expect(wrapper.text()).toBe('old render + old state');

spy.reset();
{
class App extends Component {
componentWillMount() {
this.state = 'new';
}

shouldComponentUpdate() {
return false;
}

handleClick = () => {
spy('bar');
};

render() {
return (
<span onClick={this.handleClick}>new render + {this.state} state</span>
);
}
}
RHL.register(App, 'App', 'test.js');
wrapper.setProps({ children: <App /> });
}

wrapper.find('span').simulate('click');
expect(spy).toHaveBeenCalledWith('bar');
expect(wrapper.text()).toBe('new render + old state');
});

it('replaces children class property arrow functions without block statement bodies', () => {
const spy = createSpy();

class App extends Component {
componentWillMount() {
this.state = 'old';
}

shouldComponentUpdate() {
return false;
}

handleClick = () => spy('foo');

render() {
return (
<span onClick={this.handleClick}>old render + {this.state} state</span>
);
}
}
RHL.register(App, 'App', 'test.js');

const wrapper = mount(<AppContainer><App /></AppContainer>);
wrapper.find('span').simulate('click');
expect(spy).toHaveBeenCalledWith('foo');
expect(wrapper.text()).toBe('old render + old state');

spy.reset();
{
class App extends Component {
componentWillMount() {
this.state = 'new';
}

shouldComponentUpdate() {
return false;
}

handleClick = () => spy('bar');

render() {
return (
<span onClick={this.handleClick}>new render + {this.state} state</span>
);
}
}
RHL.register(App, 'App', 'test.js');
wrapper.setProps({ children: <App /> });
}

wrapper.find('span').simulate('click');
expect(spy).toHaveBeenCalledWith('bar');
expect(wrapper.text()).toBe('new render + old state');
});

it('replaces children with class property arrow ' +
'functions with different numbers of arguments', () => {
const spy = createSpy();

class App extends Component {
componentWillMount() {
this.state = 'old';
}

shouldComponentUpdate() {
return false;
}

handleClick = () => spy('foo');

render() {
return (
<span onClick={this.handleClick}>old render + {this.state} state</span>
);
}
}
RHL.register(App, 'App', 'test.js');

const wrapper = mount(<AppContainer><App /></AppContainer>);
wrapper.find('span').simulate('click');
expect(spy).toHaveBeenCalledWith('foo');
expect(wrapper.text()).toBe('old render + old state');

spy.reset();
{
class App extends Component {
componentWillMount() {
this.state = 'new';
}

shouldComponentUpdate() {
return false;
}

handleClick = ({ target }) => spy(target.value);

render() {
return (
<span onClick={this.handleClick}>new render + {this.state} state</span>
);
}
}
RHL.register(App, 'App', 'test.js');
wrapper.setProps({ children: <App /> });
}

wrapper.find('span').simulate('click', { target: { value: 'bar' } });
expect(spy).toHaveBeenCalledWith('bar');
expect(wrapper.text()).toBe('new render + old state');
});
});

describe('with createClass root', () => {
Expand Down
3 changes: 3 additions & 0 deletions test/babel/fixtures/class-properties/block-body/.babelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"plugins": ["syntax-class-properties", "../../../../../src/babel"]
}
5 changes: 5 additions & 0 deletions test/babel/fixtures/class-properties/block-body/actual.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class Foo {
bar = (a, b) => {
return a(b);
};
}
21 changes: 21 additions & 0 deletions test/babel/fixtures/class-properties/block-body/expected.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
var _this = this;

class Foo {
bar = (...params) => _this.__bar__REACT_HOT_LOADER__(...params);
Copy link

Choose a reason for hiding this comment

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

Just noticed this line.
Wouldn't a call to foo.bar() be forwarded to window.__bar__REACT_HOT_LOADER__ instead of the class instance? Since the first line binds _this to the global object and not the class instance?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@janv I'd noticed that before and thought it was weird too (like, how would it work at all if it was really outputting this code?), but that's apparently what happens when you use babel with only babel-plugin-syntax-class-properties, without the corresponding transform plugin. (which we're doing in these test fixtures). I'll fix these tests to use both plugins so that this confusing output doesn't confuse anyone in the future.


__bar__REACT_HOT_LOADER__(a, b) {
return a(b);
}

}
;
Copy link
Collaborator

Choose a reason for hiding this comment

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

not related with this PR I think but what's up with this ; and the other one at the end?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assumed it was Babel weirdness, I was making the tests pass here. According to AST explorer (highlight the semicolon) that's an additional empty statement, I'll see if I'm accidentally creating one.


(function () {
if (typeof __REACT_HOT_LOADER__ === 'undefined') {
return;
}

__REACT_HOT_LOADER__.register(Foo, "Foo", __FILENAME__);
})();

;
3 changes: 3 additions & 0 deletions test/babel/fixtures/class-properties/default-params/.babelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"plugins": ["syntax-class-properties", "../../../../../src/babel"]
}
5 changes: 5 additions & 0 deletions test/babel/fixtures/class-properties/default-params/actual.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class Foo {
bar = (a = "foo") => {
return `${a}bar`;
};
}
21 changes: 21 additions & 0 deletions test/babel/fixtures/class-properties/default-params/expected.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
var _this = this;

class Foo {
bar = (...params) => _this.__bar__REACT_HOT_LOADER__(...params);

__bar__REACT_HOT_LOADER__(a = "foo") {
return `${ a }bar`;
}

}
;

(function () {
if (typeof __REACT_HOT_LOADER__ === 'undefined') {
return;
}

__REACT_HOT_LOADER__.register(Foo, "Foo", __FILENAME__);
})();

;
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"plugins": ["syntax-class-properties", "../../../../../src/babel"]
}
Loading