React.unstable_AsyncComponent by acdlite · Pull Request #10239 · facebook/react (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation24 Commits1 Checks0 Files changed
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
Alternative to using the static class property unstable_asyncUpdates. Everything inside has async updates by default.
Should this be AsyncComponent maybe?
I imagine people would conflate it with React.Component and React.PureComponent and try to do
class MyComponent extends React.AsyncComponent {...}
or is that what you meant?
We could call it AsyncWrapper to make it a bit more explicit. That's what I named the internal module.
It's not that bad if they do use the inheritance. They could use either.
True but that prevents us in the future from changing this from a class to something else.
But I guess that's not that important. I do like the symmetry with the other component exports.
It also solves the problem of the return value of ReactDOM.render being different because you can just use inheritance to make the root async.
Clever! Ok you've convinced me
There is a problem using static flags though. We didn't do that for isReactComponent because Scala JS (and probably others) don't support static flags on classes so they didn't inherit those flags. Instead we put the flag on the prototype. We should probably do that instead.
It should probably also be an object instead of boolean. We did that because jest automocking sometimes screwed up the mocking the boolean as undefined. I'm not sure if that's a big problem anymore but could be.
Interesting... ok. Is there a reason we originally went with a static flag for unstable_asyncUpdates? I can't remember.
There is no special syntax for adding a flag to a prototype (other than methods) and it's inefficient to add it to every instance instead of shared on the prototype. That's solved by the base class model.
| const ReactBaseClasses = require('ReactBaseClasses'); |
| class ReactAsyncWrapper extends ReactBaseClasses.Component { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we go with very minimal function prototype thing rather than Babel class output? We’ve tried very hard to trim down isomorphic React package, class output was a bit bloated last I checked.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About to push a commit that does this the same way as PureComponent
React.unstable_Async React.unstable_AsyncComponent
| this.updater = updater || ReactNoopUpdateQueue; | | ------------------------------------------------ | | } | | | | function ComponentDummy2() {} |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse the dummy function from above? Not sure if that is worth from an optimization perspective or not.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| this.updater = updater || ReactNoopUpdateQueue; | | ----------------------------------------------------- | | } | | | | ReactAsyncComponent.prototype = new ComponentDummy(); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth adding a variable here. var ReactAsyncComponentPrototype = here so that you can use that instead of rereading the prototype below. Save a few bytes.
| ReactAsyncComponent.prototype.constructor = ReactAsyncComponent; |
|---|
| // Avoid an extra prototype jump for these methods. |
| Object.assign(ReactAsyncComponent.prototype, ReactComponent.prototype); |
| ReactAsyncComponent.prototype.unstable_isAsyncReactComponent = true; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do = {} just like isReactComponent does?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it the same as PureComponent. I don't have all the context for why this is important, but if it is, we should do it for PureComponent as well.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. It might not be noticeable for PureComponent because nobody relies on shouldComponentUpdate for behavior.
However, worst case, I guess this just triggers sync mode.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, the Jest thing. Yeah, funny that the "worst case" in this situation is exactly what we'd want it to be, anyway, per our recent discussion. :D
Alternative to using the static class property unstable_asyncUpdates. Everything inside has async updates by default. You can also extend it like PureComponent or Component.
@acdlite
Is there any way to use this feature?
Currently, the feature is in behind the flag ReactFeatureFlags.enableAsyncSubtreeAPI so I can't use this although the API is exposed as unstable_AsyncComponent.
I can use this feature by modifying the flag in node_modules/react-dom directly but I want to avoid it.
What reason is the feature in behind the flag?
I think it would be nice if I can use this as an opt-in feature without the above way.
I've missed that the flag is already true.
https://github.com/facebook/react/blob/master/src/renderers/shared/utils/ReactFeatureFlags.js#L16
I've misunderstood unstable_AsyncComponent, Top level unstable_AsyncComponent and ReactDOM.flushSync() work fine for me 👏
I tried to wrap some heavy components(many list components) by unstable_AsyncComponent and update by setState from a root component.
I expected to update wrapped components asynchronously(with rIC) and others synchronously but the update seemed to be all synchronous.
