Conversation
bvaughn
changed the title
index.js
Outdated
| var prevState = this.state; | ||
| this.props = nextProps; | ||
| this.state = nextState; | ||
| this.__reactInternalSnapshot = this.getSnapshotBeforeUpdate( |
There was a problem hiding this comment.
This would probably need to be in try/finally if we do this. In case it throws.
There was a problem hiding this comment.
Rats. I meant to add that and forgot. 😄
If we move forward with this PR (which I doubt) I'll add it.
…SnapshotBeforeUpdate
bvaughn
changed the title
|
I've updated the README to specify which lifecycles are supported, and to link to React docs for more information instead of duplicating. Even if we don't move forward with this PR, I'll probably keep most of the README changes. |
README.md
Outdated
|
|
||
| Note that in order for the polyfill to work, none of the following lifecycles can be defined by your component: `componentWillMount`, `componentWillReceiveProps`, or `componentWillUpdate`. | ||
|
|
||
| Note also that if your component contains `getSnapshotBeforeUpdate`, `componentWillUpdate` must be defined as well. |
There was a problem hiding this comment.
Did you mean componentDidUpdate?
There was a problem hiding this comment.
In other words I'd expect the person trying to use getSnapshotBeforeUpdate to want to migrate away from componentWillUpdate, not use it.
There was a problem hiding this comment.
Yes 😦 This was a typo.
bvaughn
changed the title
| var prevState = this.state; | ||
| this.props = nextProps; | ||
| this.state = nextState; | ||
| this.__reactInternalSnapshot = this.getSnapshotBeforeUpdate( |
There was a problem hiding this comment.
This is the kind of thing for which a WeakMap would be good for... but can't use WeakMap in React :(
Have you tried at least making it non-enumerable, or even maybe also a polyfill-internal Symbol instead of a string name, or are those also out when supporting 0.14?
There was a problem hiding this comment.
I don't think that's necessary. React already stores other "__reactInternal" attributes on class components. And relying on a symbol might cause problems for older browsers (supported by older versions of React) that don't support symbols.
There was a problem hiding this comment.
Would be nice if a future version of React would use a WeakMap for their per-instance internal state instead (React 17?) 🤔
| var prevProps = this.props; | ||
| var prevState = this.state; | ||
| this.props = nextProps; | ||
| this.state = nextState; |
There was a problem hiding this comment.
Won't direct assignments to these warn?
There was a problem hiding this comment.
No. React sets/updates this values in a similar way before calling lifecycles.
|
We reached consensus today on moving forward with this polyfill approach- so now I just need a review 😄 |
index.js
Outdated
| } | ||
| if (typeof Component.prototype.componentDidUpdate !== 'function') { | ||
| throw new Error( | ||
| 'Cannot polyfill getSnapshotBeforeUpdate() for components that do not define componentDidUpdate()' |
There was a problem hiding this comment.
Maybe add "on the prototype" to these messages?
gaearon
left a comment
gaearon
left a comment
There was a problem hiding this comment.
Looks good. Would be nice to have a test that more closely mimics the intended usage (e.g. that uses refs).
|
Fair enough. Updated! |
3 participants
Potential polyfill for
getSnapshotBeforeUpdate(RFC: reactjs/rfcs/pull/33, implementation: facebook/react/pull/12404).Note that if we did use this approach, we'd need to update React to also look for a(Added to facebook/react/pull/12404)__suppressDeprecationWarningflag on thecomponentWillUpdatemethod before logging a DEV warning.I'm currently on the fence about whether we should polyfill
getSnapshotBeforeUpdateusing this technique. It would be useful for libs that rely on it, so they could avoid having to break backwards compatibility with supported React versions.