Propagate errors while resolving lazy() default export #21639
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: 0eea577...b98e967 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
|
|
Hmm. Types say we care about using a minimal |
|
OK so there's actually two separate scenarios that could happen:
Ideally both would show good messages. |
packages/react/src/ReactLazy.js
Outdated
| }, | ||
| ); | ||
| if (__DEV__) { | ||
| if (defaultExport === undefined) { |
There was a problem hiding this comment.
Why isn't this also a Rejected case?
There was a problem hiding this comment.
What do you mean? It gets Rejected later on throw the catch. This particular check is just to give a nicer message but there could be more reasons .default access throws (e.g. a getter that throws) so I wanted to separate out the DEV message for one particular case and the production check.
There was a problem hiding this comment.
Sorry, to clarify, if moduleObject.default doesn't throw, but returns undefined for some other reason (no default export) then we'd consider it Resolved. Is that "supported"?
There was a problem hiding this comment.
I think that's "supported" in the sense that it's going to throw in reconciler later when we try to render it. The reconciler knows what it can or cannot render. So we don't need to exhaustively check it here.
Whereas if it's loading itself that fails, we don't even get to the reconciler. Because we end up caching undefined instead of a Promise, and keep throw undefined.
There was a problem hiding this comment.
I guess...but it feels weird to not handle it here, where we know the cause.
There was a problem hiding this comment.
I wanted to avoid adding duplicate production checks in a hot path since we're going to check the same thing later. So the only places I'm adding checks to are the ones we would not check later because they're be incorrectly flagged as Pending. This is more about fixing the state machine of Lazy than about fixing what can or cannot get rendered.
|
So I reshuffled the code a bit to also catch errors caused by thenable not being a thenable. I'm not super happy with the structure but happy to take suggestions if maybe there's an easier way to do it. The main thing I want to solve is that we should never have a |
| } | ||
| try { | ||
| defaultExport = moduleObject.default; | ||
| } catch (e) { |
There was a problem hiding this comment.
This is for a lazy(async () => { import(...) }) case.
| ); | ||
| try { | ||
| thenable.then(onFulfill, onReject); | ||
| } catch (e) { |
There was a problem hiding this comment.
This is for a lazy(() => { import(...) }) case.
|
We should move the undefined check to the reconciler because this is now valid: It doesn't currently affect Flight because it uses its own resolver but it's valid in the API to do this in user space. Edit: Correction. Only the check for undefined when the promise resolves to undefined. Not returning a Promise at all is probably not best practice regardless. However it many places where Promises are used you can use either the value or a Promise and this would be one where it's not which is awkward for types. Which is why we won't do this at runtime anymore for return values in render. Edit 2: I guess this is not valid unless it's wrapped in a Edit 3: Actually it is an issue for the warning. |
|
I posted another take: #21642 |
I think this might fix #18768. I haven't tested yet. Basically, the problem is that the Promise has resolved but we never record that because the handler that's meant to change the status throws.
My fix is to make the error handler catch that. I could also change theI changed it to just catch this part specifically with a try/catch.thenbranch itself but didn't want to touch the common case.