Skip to content
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

React.unstable_AsyncComponent #10239

Merged
merged 1 commit into from Jul 21, 2017
Merged

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jul 20, 2017

Alternative to using the static class property unstable_asyncUpdates. Everything inside has async updates by default.

@sebmarkbage
Copy link
Collaborator

Should this be AsyncComponent maybe?

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 20, 2017

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?

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 20, 2017

We could call it AsyncWrapper to make it a bit more explicit. That's what I named the internal module.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Jul 20, 2017

It's not that bad if they do use the inheritance. They could use either.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 20, 2017

True but that prevents us in the future from changing this from a class to something else.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 20, 2017

But I guess that's not that important. I do like the symmetry with the other component exports.

@sebmarkbage
Copy link
Collaborator

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.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 20, 2017

Clever! Ok you've convinced me

@sebmarkbage
Copy link
Collaborator

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.

@sebmarkbage
Copy link
Collaborator

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.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 20, 2017

Interesting... ok. Is there a reason we originally went with a static flag for unstable_asyncUpdates? I can't remember.

@sebmarkbage
Copy link
Collaborator

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 {
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

@acdlite acdlite changed the title React.unstable_Async React.unstable_AsyncComponent Jul 20, 2017
this.updater = updater || ReactNoopUpdateQueue;
}

function ComponentDummy2() {}
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@acdlite acdlite force-pushed the React.unstable_Async branch 2 times, most recently from f92982f to 229195b Compare July 20, 2017 23:56
this.updater = updater || ReactNoopUpdateQueue;
}

ReactAsyncComponent.prototype = new ComponentDummy();
Copy link
Collaborator

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;
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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 <AsyncComponent /> has async updates by default. You
can also extend it like PureComponent or Component.
@acdlite acdlite merged commit ae430b7 into facebook:master Jul 21, 2017
@koba04
Copy link
Contributor

koba04 commented Sep 16, 2017

@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.

@koba04
Copy link
Contributor

koba04 commented Sep 19, 2017

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.

<div>
  <Other />
  <AsyncComponent>
    <HeavyList />
  </AsyncComponent>
</div>

@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented Sep 20, 2017

fiberAsyncScheduling: false,

I thinks maybe that's because above flag is false?But I find:

// Check if the top-level element is an async wrapper component. If so, treat
// updates to the root as async. This is a bit weird but lets us avoid a separate
// `renderAsync` API.
const forceAsync =
ReactFeatureFlags.enableAsyncSubtreeAPI &&
element != null &&
element.type != null &&
element.type.prototype != null &&
(element.type.prototype: any).unstable_isAsyncReactComponent === true;

It seems that just enableAsyncSubtreeAPI is true can works. And from

it('can opt-in to async scheduling inside componentDidMount/Update', () => {
we can do this in cDM/cDU.

And IIRC, async render will be enabled at least react-17, so I am not sure why they plan to do this just now.

@acdlite ping~

@gaearon gaearon mentioned this pull request Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants