Refactor Lazy Components to use teh Suspense (and wrap Blocks in Lazy) #18362
Conversation
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
sebmarkbage
commented
Mar 21, 2020
| let payload = lazyComponent._payload; | ||
| let init = lazyComponent._init; | ||
| try { | ||
| return getComponentName(init(payload)); |
There was a problem hiding this comment.
Getting the component name can now have side-effects, causing data to load.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 8a6c32b:
|
Details of bundled changes.Comparing: 31a9e39...8a6c32b react
react-dom
react-art
react-test-renderer
react-native-renderer
react-reconciler
ReactDOM: size: -1.3%, gzip: -1.2% React: size: 🔺+2.7%, gzip: 🔺+2.4% Size changes (experimental) |
Details of bundled changes.Comparing: 31a9e39...8a6c32b react
react-dom
react-native-renderer
react-art
react-test-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: -0.0% React: size: 🔺+2.4%, gzip: 🔺+2.2% Size changes (stable) |
sebmarkbage
commented
Mar 21, 2020
| if (element.type.$$typeof === REACT_BLOCK_TYPE) { | ||
| let type = element.type; | ||
| if (type.$$typeof === REACT_LAZY_TYPE) { | ||
| type = resolveLazyType(type); |
There was a problem hiding this comment.
For updates, we attempt to initialize earlier to see if we can reconcile against the inner value. I only do this for Blocks right now to preserve backwards compatibility.
However, in non-legacy mode, we could reconcile against the resolved value of any type.
E.g. if I have two lazy components that resolve to the same thing, why shouldn't they reconcile against each other?
d26721f to
5b2c1c9
Compare
4250bb1 to
aa99216
Compare
gaearon
approved these changes
Mar 22, 2020
| lazyComponent: LazyComponent<T, P>, | ||
| ): LazyComponent<T, P> | T { | ||
| try { | ||
| // If we can, let's peak at the resulting type. |
| try { | ||
| outerMemoType = init(payload); | ||
| } catch (x) { | ||
| outerMemoType = null; |
There was a problem hiding this comment.
Why not just let it throw? If we got here, doesn't mean it's resolved anyway?
There was a problem hiding this comment.
Hm. I think you’re right. I think that we’d only get here if the init is not deterministic. I.e. if it resuspends. That’s a bug in the lazy code but could possibly happen as a bug in a “user provided lazy”.
That would be a prod changing behavior so probably best to keep this since it wouldn’t happen in prod (although the call wouldn’t happen neither).
| @@ -879,7 +879,13 @@ describe('ReactLazy', () => { | |||
| }, | |||
| ); | |||
|
|
|||
| expect(Scheduler).toFlushAndYield(['Started loading', 'Loading...']); | |||
| if (__DEV__) { | |||
| // Getting the name for the warning cause the loading to start early. | |||
There was a problem hiding this comment.
Don't know how I feel about this. Can you motivate?
There was a problem hiding this comment.
I think we really should be refactoring the getComponentName stuff. One todo I have is to pass it a fiber because the type is not really enough. If we had the fiber we could read the .type off of that.
It’s called everywhere though so it’s some work to properly clean that up.
What annoys be a bit more is that we have that logic in prod. I suspect that’s what leads this PR to increase bundle size but just a little bit.
We probably should fork getComponentName and accompanying logic into specific DEV and prod versions for different use cases.
Regardless, I’m confident the fix is in the logging and not compromising the logic for lazy resolution.
Then resolve to a true Block inside.
sebmarkbage
commented
Nov 30, 2020
| initializeBlockComponentType(element.type); | ||
| let type = element.type; | ||
| if (type.$$typeof === REACT_LAZY_TYPE) { | ||
| type = resolveLazyType(type); |
There was a problem hiding this comment.
This got lost when we dropped Blocks. The benefit of Blocks was that we only needed this check if the tag was Block but now we need to do it for everything.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I wasn't happy with my approach in #18220
Exposing the status stuff leaks a lot of complexity downstream. It avoids taking on a virtual dispatch but also means that other code (like Flight and Blocks) can't take advantage of what Lazy already does.
Using the suspense protocol is actually really nice in terms of code size and complexity. The principle is simple:
The lazy component type can now be used to implement all kinds of lazy behavior. So I also use it to implement lazy Blocks.