-
Notifications
You must be signed in to change notification settings - Fork 557
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
async-safe, static lifecycle hooks #6
Merged
Merged
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
9818f57
Initial proposal for async-friendly, static lifecycle hooks
bvaughn 839547f
Added a note about the prefix
bvaughn b7189cc
Made deprecation/rename clearer
bvaughn aad6845
Hopefully clarified the computeMemoizeData() placeholder example
bvaughn 99988da
Renamed example method for improved clarity
bvaughn 4a34d25
Added note about aEL being unsafe in cWM
bvaughn 902d4b3
Fixed typo
bvaughn 3df7ce6
Fixing typo
bvaughn 1864c93
Removed Facebook-specific terminology
bvaughn 428758b
Tweaked wording for clarity
bvaughn cfcb7e8
Reorganization (part 1, more to come)
bvaughn 536084d
Added some more focused examples
bvaughn a1431a4
Added a comment about calling setState on a possibly unmounted component
bvaughn 3c6132e
Cancel async request on unmount in example
bvaughn 4425dbe
Tweaking examples based on Dan's feedback
bvaughn 67272ce
Renamed deriveStateFromProps to getDerivedStateFromNextProps
bvaughn c7f6728
Renamed prefetch to optimisticallyPrepareToRender
bvaughn bb2d246
Added `render` to a list
bvaughn 8618f70
Removed static optimisticallyPrepareToRender() in favor of render()
bvaughn 8f6c20e
Renamed getDerivedStateFromNextProps to getDerivedStateFromProps
bvaughn e05e317
Updated when getDerivedStateFromProps is called and what its argument…
bvaughn 7042a2a
Renamed unsafe_* to UNSAFE_*
bvaughn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bvaughn The PR for React included
a note that
getDerivedStateFromProps
doesn't receive previous props. Should this sentence be updated?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording here is a bit ambiguous but I think that's okay. Current and previous props can be compared, as shown in this example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, previous props in this case means props that have been synced to the local state and can be access via
prevState
, right? Alright then, I thought this was left over since I think theprevProps
parameter got removed in a later iteration.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So @bvaughn does this mean that, given our previous conversation, I have to "sync" all my props to state in order to compare previous props with the new props? Feels like I'm missing something here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all
props
, only ones that are directly used to derivestate
, and only if it's an expensive computation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit different though. You're not storing a "mirrored" version, you're storing the previous version. Which is a special case of an accumulation (that happens to completely discard the current value). Accumulation is exactly the purpose of
getDerivedStateFromProps
and since you already added it (presumably for accumulating something), adding another field to it doesn't hurt.I guess one could say that a "mirrored" version would also be a special case of accumulation (that discards the previous value) but that's sophism IMO :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I see what you mean. I just feel that it muddies the message of "don't put unnecessary stuff in state" where "necessary" no longer precisely means "used in
render
". Personally I find this a bigger / less intuitive exception to a rule thanprevProps
's nullability (the reason for which is pretty obvious -- noprevProps
on first render). I suspect newer devs would struggle more with the former.Anyway I see the trade-off, and I appreciate your willingness to explain! 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might actually revisit this one ^^
With async React it might not be safe to put certain things on the instance fields. See facebook/react#10580.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bvaughn @gaearon I've tried to flesh out what I mean using a Typescript example - see this gist - to compare the two approaches.
I've extracted this example from a real life component that has more props, but hopefully you get the idea: any component would have to follow a similar pattern.
There are three files in the gist:
static getDerivedStateFromProps(nextProps: Props, prevProps: Props, initial: boolean): State
Notice file 2 is using a slightly different signature to the proposed in my last response; I agree with @gaearon the nullability issue is best avoided, hence
prevProps
is not nullable and an explicitboolean
is provided to indicate whether it's the initial derivation or not.Does the diff help to show what I mean about the boilerplate required when we have to "sync" to state?
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous props would still need to be null or undefined in this case, no? Or even more confusingly, they would equal current props.
Anyway~ thanks for the feedback, Paul. I understand and appreciate your concern about verbosity. 🙇
This proposal was discussed in length in December, accepted, and merged into the React repo a couple of weeks ago. I'm going to lock down the thread at this point because I think we've already committed to this API.
Hope you understand!