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

Support sync thenables for lazy() #14626

Merged
merged 2 commits into from Jan 18, 2019

Conversation

Projects
None yet
4 participants
@gaearon
Copy link
Member

gaearon commented Jan 18, 2019

Why don't we? Currently they fail with a confusing error because of a race condition (status gets set to resolved but then the result gets overwritten by the next line). This should fix it.

I figured this might be useful for testing. See airbnb/enzyme#1917 (comment). It's awkward that people look for workarounds like waitForLazyLoaded in a synchronous environment. Supporting sync thenables could be a nice solution to that.

@acdlite
Copy link
Member

acdlite left a comment

Yeah we should do this but let's avoid the redundant commit

</Suspense>,
);

expect(ReactTestRenderer).toHaveYielded(['Loading...', 'Hi']);

This comment has been minimized.

@acdlite

acdlite Jan 18, 2019

Member
Suggested change Beta
expect(ReactTestRenderer).toHaveYielded(['Loading...', 'Hi']);
expect(ReactTestRenderer).toHaveYielded(['Hi']);
lazyComponent._result = thenable;
// Don't race with a sync thenable
if (lazyComponent._status === Pending) {
lazyComponent._result = thenable;

This comment has been minimized.

@acdlite

acdlite Jan 18, 2019

Member

Alt suggestion:

// Check if it resolved synchronously
if (lazyComponent._status === Resolved) {
  return lazyComponent._result;
}

Since if it's already resolved we don't need to throw the thenable

This comment has been minimized.

@gaearon

gaearon Jan 18, 2019

Author Member

Aaaah. That's why it had the loading state. Thanks.

@@ -47,6 +47,7 @@ export function readLazyComponentType<T>(lazyComponent: LazyComponent<T>): T {
lazyComponent._status = Pending;
const ctor = lazyComponent._ctor;
const thenable = ctor();
lazyComponent._result = thenable;

This comment has been minimized.

@gaearon

gaearon Jan 18, 2019

Author Member

This is still needed because we read thenable from lazyComponent._result while it's pending. If we keep the assignment, it will overwrite sync thenables. But if we remove it at all, we will throw null instead of thenable. This fixes both cases.

This comment has been minimized.

@acdlite

acdlite Jan 18, 2019

Member

You still need the assignment, but I don't think you need to move it up here. If you keep it right after the early return branch you added on line 78-80, then there's always a single assignment.

if (lazyComponent._status === Resolved) {
  return lazyComponent._result;
}
lazyComponent._result = thenable;
throw thenable;
// Check if it resolved synchronously
if (lazyComponent._status === Resolved) {
return lazyComponent._result;
}

This comment has been minimized.

@gaearon

gaearon Jan 18, 2019

Author Member

We could also do the same for failure. But doesn't seem worth the extra code to me. Could be a recursive call but then there's more risk of messing it up and going into a stack overflow. Meh.

@gaearon

This comment has been minimized.

Copy link
Member Author

gaearon commented Jan 18, 2019

I'll merge since this follows your suggestion.

@gaearon gaearon merged commit 9120f6c into facebook:master Jan 18, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@gaearon gaearon referenced this pull request Jan 18, 2019

Open

Support Suspense #1917

2 of 13 tasks complete

@gaearon gaearon deleted the gaearon:then-sync branch Jan 18, 2019

jetoneza added a commit to jetoneza/react that referenced this pull request Jan 23, 2019

Support sync thenables for lazy() (facebook#14626)
* Support sync thenables for lazy()

* Don't commit twice
@ntucker

This comment has been minimized.

Copy link

ntucker commented Jan 30, 2019

Does this make lazy() work in SSR?

@gaearon

This comment has been minimized.

Copy link
Member Author

gaearon commented Jan 30, 2019

No, <Suspense> is still unsupported. You’d also get a hydration mismatch if your server renders everything but the client lazy loads.

n8schloss added a commit to n8schloss/react that referenced this pull request Jan 31, 2019

Support sync thenables for lazy() (facebook#14626)
* Support sync thenables for lazy()

* Don't commit twice

Kiku-git added a commit to Kiku-git/react that referenced this pull request Feb 10, 2019

Support sync thenables for lazy() (facebook#14626)
* Support sync thenables for lazy()

* Don't commit twice
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment