Add transform to support arrow function class properties #322
Conversation
properly when the method is changed, as well as a failing test demonstrating that class properties do not get replaced.
…nctions into hot-reloadable class methods (gaearon#242)
…ing tests. also restructure the babel plugin tests
|
Ok, I fixed the case for arrow functions without block statement bodies. I also found a case where it failed, because the |
|
Wow! Thanks for working on this. |
|
Awesome work @calesce ! We tried this out and found it broke when we had methods with default params. It seemed to work fine in all the other cases in our project. Example: class defaultParamsComponent extends React.Component {
defaultParam = (param = 'test') => {
return param
}
render() {
return (
<div>
{this.defaultParam()}
</div>
)
}
}Gives the error
@gaearon We'd love to see this implemented. Hot-reloading is pretty broken for us because we are using arrow function methods. |
|
Thanks for the feedback @ro-savage! I also just noticed it breaks when destructuring arguments, eg: |
properties transform, since the parameters in the source code can have default values, which don't need to be copied when calling the new generated method
if the class properties function has destructured parameters, instead of copying the destructuring expression, create new identifiers for each param, and call the generated method with those new parameters
|
@calesce I updated our react-hot-loader with your latest changes and its working perfectly for us. Thanks heaps! |
|
@ro-savage no problem! I now realize that this type of transform won't work if a developer were to change the number of arguments when changing a class property function, since the argument count won't be reloaded. Which isn't good. I think something like this might work instead: before: class App {
foo = (bar, baz) => {
return bar + baz
}
}after: class App {
foo = (...params) => {
return this.__foo__REACT_HOT_LOADER__(...params)
}
__foo__REACT_HOT_LOADER__(bar, baz) {
return bar + baz
}
}That way, the number of arguments won't matter, so the developer can add/remove args and it should reload fine. Edit: |
This allows the class property functions to be hot-reloaded properly when changing the number of arguments
|
I tried your fork, and it works on a small app, but not on a larger application. I'm switching back to RHL1.3 for the moment. I hope you figure it out. Would love to help, but this is a bit over my head ATM. |
|
Hey thanks! Any details on where/how it doesn't work? |
|
@calesce will test it again tomorrow and report. Basically all I need is class arrow functions to hot-reload. I use them heavily throughout my app and without them being hot-reloaded is pretty much useless for me. For now I switched to binding them in constructor, and now they do reload, so I don't have to reload the app anymore :) |
|
@HriBB, can you find out what is not hot reloading? I am using it on an app that solely uses arrow functions and contains >200 components and haven't ran into any problems. |
|
@calesce @ro-savage I must be doing something wrong, since even basic hot reload does not work. I'm not sure if I installed @calesce's fork properly. This is what I did. App loads fine, everything works except hot reload. I am using When I change some file, I get the standard output in the console, but nothing happens. But nothing is changed. It works as expected with npm version of Any ideas? I'm looking at the RHL source, but I'm not yet familiar with it and so I have no idea how to debug this ... This is my config |
|
Ahh I finally got it to work. I guess that @calesce @ro-savage how did you install the fork? I would really like to know so that I don't run into the same problems in the future. It looks like it's working. I will do some more testing over the next few days and report any problems here ;) |
|
@HriBB: I think you needed to The way I tested was copy-pasting the built files into my app's
|
|
@calesce yeah I know. It only worked if I manually copied files into my project's If I used the This is out of the scope of this PR, but does anyone know how to properly link forks? |
|
Any updates on this? I tried copying the built lib folder from the react-hot-loder repo on the |
|
@carlosyasu91 It's hard to say, any way you can push up a minimal project reproducing the issue? |
| } | ||
|
|
||
| wrapper.find('span').simulate('click'); | ||
| expect(spy).toHaveBeenCalledWith('bar'); |
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
Agreed, doesn't hurt.
|
@calesce this looks awesome! what do you think of adding more isolated tests that do not include the |
|
@nfcampos thanks for the review! I agree on not needing run the ES2015/stage1 transforms on the babel fixtures, the expected output is more readable that way. |
Since the class properties proposal was moved to stage 2 at TC39, we no longer need the stage-1 dependency.
We only care about what the plugin does, and not about how Babel transpiles ES2015/class properties transforms. This makes the fixtures more immediately understandable.
verifying that the class property transform doesn't break the other transform and cause components to remount.
| } | ||
|
|
||
| } | ||
| ; |
There was a problem hiding this comment.
not related with this PR I think but what's up with this ; and the other one at the end?
There was a problem hiding this comment.
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.
opt out of class properties transpile on use of arguments, new.target inside
|
@nfcampos no problem, I think I would've have to add you to my fork's collaborators. I'll take a look at why there are additional empty statements being created tonight, then this should be good to go 👍 |
|
I have encountered an issue with this when using static class properties Processed throught the react-hot transformation, I have illustrated this in https://github.com/janv/react-hot-static-bug |
|
@janv: you're right, we're stripping the static keyword off of the properties. thanks for the repro! |
|
I ran into another issue with this. We have Components nested two levels deep, that are forwarding callbacks which we create as arrow function class properties. Something like class A {
render(){
return <B onClick={this.handleClick}/>
}
handleClick = () => this.setState(...)
}
class B {
render(){
return <button onClick={this.handleClick}/>
}
handleClick = () => this.props.onClick(...)
}
That should mean someone somewhere is keeping a reference to an original implementation of a function. I'll try to come up with a minimal executable example. Also, no sure why this only started happening after we removed babel-preset-es2015 yet |
| var _this = this; | ||
|
|
||
| class Foo { | ||
| bar = (...params) => _this.__bar__REACT_HOT_LOADER__(...params); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
|
I managed to write a minimal app that exhibits the problem: (Notice this is a new branch on the repo I used before to illustrate the issue with static class properties) |
|
Might be worth noting that we already were having problems when not using Babel to transpile classes (#313). |
|
@janv thanks again for the repro. Yeah, that definitely seems like the same problem as #313. The issue remains even if you replace the class properties in your example with normal class methods. I'm kind of surprised that "native" classes behave differently from how Babel transpiles them. The fix will probably have to come from react-proxy. |
|
@calesce yep I believe the fix would have to be in react-proxy, I played around with it on friday but haven't figured out what exactly is different about native |
|
Weirdly, it looks like |
|
@calesce yeah I noticed, not sure why the tests are not failing but this is |
I implemented this based off of the discussion in #242. I started with a failing test case where class properties didn't get hot-reloaded.
I still need to add the case for arrow functions without bodies, but I figured I'd go ahead and open up the discussion.
Also, I added a
.babelrcto theAppContainertest directory to include the Babel plugin, so I could test the class property case. While the existing test cases are still passing, I'm not sure whether running the transform will change how they work, since they rely on registering/tagging components manually.