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

Fix Issue with Undefined Lazy Imports By Refactoring Lazy Initialization Order #21642

Merged
merged 5 commits into from
Jun 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -178,13 +178,13 @@ describe('ReactLazy', () => {

await Promise.resolve();

expect(Scheduler).toFlushAndThrow('Element type is invalid');
if (__DEV__) {
expect(console.error).toHaveBeenCalledTimes(1);
expect(console.error).toHaveBeenCalledTimes(3);
expect(console.error.calls.argsFor(0)[0]).toContain(
'Expected the result of a dynamic import() call',
);
}
expect(Scheduler).toFlushAndThrow('Element type is invalid');
});

it('throws if promise rejects', async () => {
Expand Down
63 changes: 42 additions & 21 deletions packages/react/src/ReactLazy.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ type PendingPayload = {

type ResolvedPayload<T> = {
_status: 1,
_result: T,
_result: {default: T},
};

type RejectedPayload = {
Expand All @@ -53,43 +53,64 @@ 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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if it was a sync thenable though? Wouldn't you miss it due to if (payload._status === Pending) { check in onFulfill?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

pending._result = thenable;
// This might throw either because it's missing or throws. If so, we treat it
// as still uninitialized and try again next time. Which is the same as what
// happens if the ctor or any wrappers processing the ctor throws. This might
// end up fixing it if the resolution was a concurrency bug.
thenable.then(
moduleObject => {
if (payload._status === Pending) {
const defaultExport = moduleObject.default;
if (__DEV__) {
if (defaultExport === undefined) {
console.error(
'lazy: Expected the result of a dynamic import() call. ' +
'Instead received: %s\n\nYour code should look like: \n ' +
// Break up imports to avoid accidentally parsing them as dependencies.
'const MyComponent = lazy(() => imp' +
"ort('./MyComponent'))",
moduleObject,
);
}
}
if (payload._status === Pending || payload._status === Uninitialized) {
// Transition to the next state.
const resolved: ResolvedPayload<T> = (payload: any);
resolved._status = Resolved;
resolved._result = defaultExport;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh ok this is nice.

resolved._result = moduleObject;
}
},
error => {
if (payload._status === Pending) {
if (payload._status === Pending || payload._status === Uninitialized) {
// Transition to the next state.
const rejected: RejectedPayload = (payload: any);
rejected._status = Rejected;
rejected._result = error;
}
},
);
if (payload._status === Uninitialized) {
// In case, we're still uninitialized, then we're waiting for the thenable
// to resolve. Set it as pending in the meantime.
const pending: PendingPayload = (payload: any);
pending._status = Pending;
pending._result = thenable;
}
}
if (payload._status === Resolved) {
return payload._result;
const moduleObject = payload._result;
if (__DEV__) {
if (moduleObject === undefined) {
console.error(
'lazy: Expected the result of a dynamic import() call. ' +
'Instead received: %s\n\nYour code should look like: \n ' +
// Break up imports to avoid accidentally parsing them as dependencies.
'const MyComponent = lazy(() => imp' +
"ort('./MyComponent'))\n\n" +
'Did you accidentally put curly braces around the import?',
moduleObject,
);
}
}
if (__DEV__) {
if (!('default' in moduleObject)) {
console.error(
'lazy: Expected the result of a dynamic import() call. ' +
'Instead received: %s\n\nYour code should look like: \n ' +
// Break up imports to avoid accidentally parsing them as dependencies.
'const MyComponent = lazy(() => imp' +
"ort('./MyComponent'))",
moduleObject,
);
}
}
return moduleObject.default;
} else {
throw payload._result;
}
Expand Down