Fix Issue with Undefined Lazy Imports By Refactoring Lazy Initialization Order #21642
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
This is really where most unwrapping happen. The resolved promise is the module object and then we read things from it. This way it lines up a bit closer with the Promise model too since the promise resolving to React gets passed this same value. If this throws, then it throws during render so it's caught properly and you can break on it and even see it on the right stack.
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 0eea577...cc42e7b Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
gaearon
reviewed
Jun 7, 2021
| @@ -53,29 +53,17 @@ function lazyInitializer<T>(payload: Payload<T>): T { | |||
| const ctor = payload._result; | |||
| const thenable = ctor(); | |||
| // Transition to the next state. | |||
| const pending: PendingPayload = (payload: any); | |||
| pending._status = Pending; | |||
There was a problem hiding this comment.
What if it was a sync thenable though? Wouldn't you miss it due to if (payload._status === Pending) { check in onFulfill?
There was a problem hiding this comment.
Yea I fixed it here. cc42e7b
That check is really unnecessary. It's more of a safety thing in case someone has a bad thennable. We could probably remove it.
…ined Normally we'd just check if something is undefined but in this case it's valid to have an undefined value in the export but if you don't have a property then you're probably importing the wrong kind of object.
sebmarkbage
force-pushed
the
lazy-fix-prop
branch
from
1d1d5f1 to
cc42e7b
Compare
gaearon
reviewed
Jun 7, 2021
| // Transition to the next state. | ||
| const resolved: ResolvedPayload<T> = (payload: any); | ||
| resolved._status = Resolved; | ||
| resolved._result = defaultExport; |
There was a problem hiding this comment.
This change needs to be in lockstep right? Not that it affects anything but I remember some pain rolling out a change to lazy on RN. Maybe it's not hard anymore now that we check in the bundles.
There was a problem hiding this comment.
Note that this is only changing one package so there's no lockstep. This is a nice feature of the new protocol. The reconciler just calls the function and expects a value (or throw or suspense). The implementation of this is can be whatever - like Flight has a different one.
gaearon
approved these changes
Jun 7, 2021
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Jun 11, 2021
Summary: This sync includes the following changes: - **[c96b78e0e](facebook/react@c96b78e0e )**: Add concurrentRoot property to ReactNativeTypes ([#21648](facebook/react#21648)) //<Samuel Susla>// - **[1a3f1afbd](facebook/react@1a3f1afbd )**: [React Native] Fabric get current event priority ([#21553](facebook/react#21553)) //<Samuel Susla>// - **[48a11a3ef](facebook/react@48a11a3ef )**: Update next React version ([#21647](facebook/react#21647)) //<Andrew Clark>// - **[5aa0c5671](facebook/react@5aa0c5671 )**: Fix Issue with Undefined Lazy Imports By Refactoring Lazy Initialization Order ([#21642](facebook/react#21642)) //<Sebastian Markbåge>// Changelog: [General][Changed] - React Native sync for revisions 0eea577...c96b78e jest_e2e[run_all_tests] Reviewed By: bvaughn Differential Revision: D29029542 fbshipit-source-id: 9f2e19b4714b5697b5b846f2db8eb28c25420932
koto
pushed a commit
to koto/react
that referenced
this pull request
Jun 15, 2021
…ion Order (facebook#21642) * Add a DEV warning for common case * Don't set Pending flag before we know it's a promise * Move default exports extraction to render phase This is really where most unwrapping happen. The resolved promise is the module object and then we read things from it. This way it lines up a bit closer with the Promise model too since the promise resolving to React gets passed this same value. If this throws, then it throws during render so it's caught properly and you can break on it and even see it on the right stack. * Check if the default is in the module object instead of if it's undefined Normally we'd just check if something is undefined but in this case it's valid to have an undefined value in the export but if you don't have a property then you're probably importing the wrong kind of object. * We need to check if it's uninitialized for sync resolution Co-authored-by: Dan Abramov <[email protected]>
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.
This is an alternative to #21639 which might fix #18768. Needs unit tests.
We should be pretty careful about using try/catch. It doesn't come without caveats:
In general the lazy protocol is pretty simple. You just call the function and it either throws, suspends or gives you the value. It also works recursively. And since that is all happen directly on the same stack you have the full JS stack and component stack for how you got there. For example the ctor part can suspend or throw. You can also throw in the .then function potentially if it's a non-standard thenable which we support for the purpose of higher performance sync resolution.
To fix the outer resolution I simply moved the timing of when we set the flag. This is generally how you'd solve these kinds of bugs. Moving the timing of the mutation. So now we just keep it as uninitialized until we've called the thenable and so we know it's a thenable. So when we set the value, we know it'll suspend the next time we call it. This also lets this call throw and it'll also rethrow at the same place every time similar to how if the ctor throws. So you always get the right stack.
The other thing I changed is that when we resolve the
.defaultproperty in the resolving path, we don't have the right stack so it'll be tricky to debug it. Sure, this generally happens inside React so it's not a very useful stack anyway. Therefore I instead moved this resolution to when the lazy value is read. That way it just works. It's a tiny bit slower during the reread perhaps but in general the reconciler diffs the lazy wrapper and not the resolved value anyway. It's also how this whole suspense thing is supposed to work in general. Any "transform" of a value is applied during "render" after you read it. The underlying lazy thing (that is also what React "sees" in the promise passed to React) is the module object itself. Maybe it's unfortunate that this created a memory dependency on the whole module but it's supposed to be in the module cache anyway.Another thing I noticed is that we warn for
undefinedvalues but it's not invalid to import undefined from another module. E.g. Flight creates lazy values that return undefined all the time. It's not useful as an Element Type which is the previous only usage, but now it's allowed as a Node too where undefined is allowed.I changed the early warning to check for
in. We can also check forundefinedif we want specifically for element types to give a better error message but that should be checked in the reconciler where we know how we'll use it.