DropZone: ES6ify (preparing for React 15 upgrade) #5383
Conversation
ockham
force-pushed
the
update/drop-zone-es6ify
branch
3 times, most recently
from
608b6e2 to
750c848
Compare
ockham
changed the title
ockham
added
[Status] Needs Review
| @@ -8,7 +8,7 @@ var React = require( 'react' ), | |||
| * Internal dependencies | |||
| */ | |||
| var analytics = require( 'lib/analytics' ), | |||
| DropZone = require( 'components/drop-zone' ), | |||
| DropZone = require( 'components/drop-zone' ).default, | |||
There was a problem hiding this comment.
This section is small enough that it makes sense to tidy up the imports here instead. :)
There was a problem hiding this comment.
True :-) Amended the commit.
|
|
gwwar
added
[Status] Ready to Merge
and removed
[Status] Needs Review
Motivation is turning it into an ES6 class, which allows us to drop `__reactAutoBindMap` in the test and stub prototype methods directly. Required for the React 15 upgrade which drops `__reactAutoBindMap`.
ockham
force-pushed
the
update/drop-zone-es6ify
branch
from
750c848 to
b742b47
Compare
|
Thanks for reviewing! |
| componentDidMount: function() { | ||
| // Bind listeners found in componentDidMount(), except for the ones whose | ||
| // implementation doesn't refer to `this`. | ||
| this.onDrop = this.onDrop.bind( this ); |
There was a problem hiding this comment.
It's not something I've seen much conventions around, but do you think there's any value in preserving the original unbound functions and naming these differently? In newer components, I've started assigning them as this.boundFoo = this.foo.bind( this );, but I suppose there's rarely a case for needing the original (prefixing does help to indicate that it's a bound function).
There was a problem hiding this comment.
I found https://facebook.github.io/react/docs/reusable-components.html#no-autobinding, which is what I'm following here.
Since this is a refactoring, I wanted to keep method names so I wouldn't have to worry about the component's consumers.
but I suppose there's rarely a case for needing the original (prefixing does help to indicate that it's a bound function).
Yeah -- in old-style React components, the auto-binding would've overridden the original ones in all practical cases while keeping the original name. Not sure there's much value in keeping the original methods around since even with tests, the this bind seems to make only sense -- any practical examples for using the unbound methods you can think of?
There was a problem hiding this comment.
any practical examples for using the unbound methods you can think of?
No, not off the top of my head. And even if there were, I suppose it could be as simple as applying another bind to the already-bound function?
I wasn't aware of the React documentation including that as an example, and you make a good point that this replicates the behavior of React.createClass. Then again, one could argue that the behavior of React.createClass is not one that should be replicated 😄 (given that it's not obvious at a glance how the functions are bound)
|
Delayed review, but looks good 👍 |
Motivation is to turn it into an ES6 class, which allows us to drop
__reactAutoBindMapin the test and stub prototype methods directly.Required for the React 15 upgrade which drops
__reactAutoBindMap.(See #5116 for the upgrade PR.)
To test -- check that the
DropZonecomponent works as before (try http://calypso.localhost:3000/devdocs/design/dropzones). Also, make sure that allrequire( './drop-zone' )instances now have.defaultattached, due to the ES6 export. (I only found one occurrence, inMediaDropZone.)/cc @aduth for review