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

React.lazy constructor must return result of a dynamic import #13886

Merged
merged 1 commit into from
Oct 19, 2018

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Oct 19, 2018

Based on #13885

We may want to change the protocol later, so until then we'll be restrictive. Heuristic is to check for existence of default.

See #13885 for more details.

@sizebot
Copy link

sizebot commented Oct 19, 2018

Details of bundled changes.

Comparing: d9659e4...4a5f5cc

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

@sebastiankemp
Copy link

I am confused why this is linked to my issue?

@acdlite
Copy link
Collaborator Author

acdlite commented Oct 19, 2018

@sebastiankemp Copypasta

@acdlite acdlite force-pushed the react-lazy-import branch 4 times, most recently from d983636 to c91cb61 Compare October 19, 2018 02:47
@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented Oct 19, 2018

If this is just for Component(from the codes and naming like getResultFromResolvedLazyComponent), should we add a check for the default field rather than just check whether it is undefined?

We may want to change the protocol later, so until then we'll be
restrictive. Heuristic is to check for existence of `default`.
@acdlite acdlite merged commit d9a3cc0 into facebook:master Oct 19, 2018
@arianon
Copy link

arianon commented Oct 19, 2018

Doesn't this make the case of importing a lazy component that isn't a module's default export a bit less obvious?

Before

const FooComponent = React.lazy(() =>
  import("component-library").then(module => module.FooComponent)
);

After

const FooComponent = React.lazy(() =>
  import("component-library").then(module => ({
    default: module.FooComponent
  }))
);

@milesj
Copy link
Contributor

milesj commented Oct 19, 2018

Why not check for __esModule and then choose default? That would support Babel/TS compiled files out of the box.

// Babel exported default
if (typeof result === 'object' && result.__esModule && result.default) {
  return result.default;
}

// module.exports default
return result;

@gaearon
Copy link
Collaborator

gaearon commented Oct 19, 2018

Please comment on the RFC if you have feedback. It contains the motivation and more details: reactjs/rfcs#64

acdlite added a commit to acdlite/react that referenced this pull request Oct 19, 2018
Fixes a bug where a lazy component does not cache the result of
its constructor.
acdlite added a commit to acdlite/react that referenced this pull request Oct 19, 2018
Fixes a bug where a lazy component does not cache the result of
its constructor.
acdlite added a commit to acdlite/react that referenced this pull request Oct 19, 2018
Fixes a bug where a lazy component does not cache the result of
its constructor.
acdlite pushed a commit that referenced this pull request Oct 19, 2018
Fixes a bug where a lazy component does not cache the result of
its constructor.
linjiajian999 pushed a commit to linjiajian999/react that referenced this pull request Oct 22, 2018
…ok#13886)

We may want to change the protocol later, so until then we'll be
restrictive. Heuristic is to check for existence of `default`.
linjiajian999 pushed a commit to linjiajian999/react that referenced this pull request Oct 22, 2018
Fixes a bug where a lazy component does not cache the result of
its constructor.
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
…ok#13886)

We may want to change the protocol later, so until then we'll be
restrictive. Heuristic is to check for existence of `default`.
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
Fixes a bug where a lazy component does not cache the result of
its constructor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants