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() #64

Merged
merged 3 commits into from Oct 23, 2018

Conversation

@gaearon
Member

gaearon commented Oct 19, 2018

View formatted RFC

Note: I just wrote up the RFC. The semantics are designed by @sebmarkbage and @acdlite.

@jquense

This comment has been minimized.

jquense commented Oct 19, 2018

Is the idea that the return value of lazy() MUST return a promise that resolves to {default: Component}? The downsides of that strike me as out weighing the convenience of having write lazy(async () => (await import('./Button')).default) or the same in plain promises. React almost always prefers explicit longer code to convenience so it seems a bit of an odd choice out here to require .default for what is probably(?) code that will be written not super often.

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 19, 2018

Is the idea that the return value of lazy() MUST return a promise that resolves to {default: Component}?

In the scope of this proposal, yes.

The downsides of that strike me as out weighing the convenience of having write lazy(async () => (await import('./Button')).default) or the same in plain promises.

Which downsides are you referring to?

React almost always prefers explicit longer code to convenience so it seems a bit of an odd choice out here to require .default for what is probably(?) code that will be written not super often.

I think the RFC goes into detail about why we specifically want access to the module object. This will be important for future work on the new server renderer.

An "explicit" proposal that doesn't lose this future potential would be something like

lazy(() => import('./Button'), Button => Button.default);

At this point it's going to be so common though that it might as well be the default (pun intended) behavior.

It's not just a convenience hack. It corresponds semantically to what "default" exports mean. lazy takes a module object (for reasons explained in the RFC), and picks the default export from it as it's exactly what the default export means (thing that should be read from a module when we aren't being asked something specific).

@jquense

This comment has been minimized.

jquense commented Oct 19, 2018

Which downsides are you referring to?

the inability to use named exports, or potentially do any post-import transform/instrumentation

I think the RFC goes into detail about why we specifically want access to the module object.

Sorry yeah, I can see how that would be useful, but it also feels premature? As you noted what the module object returned here even looks like is nebulous and undefined, even in Node for the SSR case. This limitation doesn't seem to really put react in a better position for the future since there is no reason why users can't do import('./Buttons').then(_ => ({ default: _.SaveButton }). Potentially breaking any future deeper integrations.

I would also add that it feels uncomfortable to me that React would want integrate so tightly into the module space, before there is a clear sense of what the standard there is. Folks do so. many. weird things in webpack, et al, to meet their needs that this is going to be a moving target for a looong time.

It's not just a convenience hack. It corresponds semantically to what "default" exports mean.

I think that's debatable, :) people assign a lot of different meaning to this stuff and i don't think that's wrong or that there is a clear "correct" usage for default vs named exports semantically

@jquense

This comment has been minimized.

jquense commented Oct 19, 2018

(do I think this a really cool addition to the API!)

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 19, 2018

the inability to use named exports, or potentially do any post-import transform/instrumentation

The proposal mentions we could add named exports to this.

Sorry yeah, I can see how that would be useful, but it also feels premature?

We're starting work on this now (both standard proposals and the new server renderer implementation) so I wouldn't say it's premature. This is going to become a very active area of work for us.

This limitation doesn't seem to really put react in a better position for the future since there is no reason why users can't do import('./Buttons').then(_ => ({ default: _.SaveButton }).

We'll document that you're supposed to pass the module object, and nothing else. For sure, some people will ignore it, but then adopting the new server renderer isn't forced upon them. People who follow the recommendation would have an easier path.

Again, if this doesn't work out, we can surely relax it in the future. In fact that's what the original proposal was (it took any Promise) but then we decided to make it stricter.

@mellogarrett

This comment has been minimized.

mellogarrett commented Oct 19, 2018

// Why don't we want to support this though?
const Button = lazy(async () => {
  const Components = await import('./components');
  // Resolve to named export:
  return components.Button;
});

I think the return statement should have a capital 'C'?

const Button = lazy(async () => {
const Components = await import('./components');
// Resolve to named export:
return components.Button;

This comment has been minimized.

@mAAdhaTTah

mAAdhaTTah Oct 19, 2018

Small typo -> Components.Button (needs capital C, or lowercase on line 69).

Edit: Looks like @mellogarrett beat me to it!

This comment has been minimized.

@Avi98

Avi98 Oct 24, 2018

i think its line 71

@sebmarkbage

This comment has been minimized.

Collaborator

sebmarkbage commented Oct 19, 2018

I think that once some of the other pieces of this puzzle fall into place. I suspect that the syntactical overhead will strongly push people towards the default exports. Especially if we can even get rid of the call to “lazy” in the default export case.

@pomber

This comment has been minimized.

pomber commented Oct 19, 2018

Nice (I'm already using it https://github.com/pomber/code-surfer-editor/blob/master/src/LazyCodeEditor.js#L4 😁).
Do you have in mind any special API or pattern for preloading? Or is the idea to preload it like any other suspenseful component (rendering it, maybe hidden)?

@matt-casey

This comment has been minimized.

matt-casey commented Oct 19, 2018

In the alternatives, it's listed as a possibility to:

Call it something long like createLazyComponent.

Has there been any more public discussion on the naming? Outside of the context of this discussion,React.lazy wouldn't necessarily be clear or self-documenting in the same way the rest of the React API is.

For example, the functions dealing with refs say so explicitly (createRef, forwardRef).
The functions dealing with components say so explicitly (createElement, cloneElement, isValidElement).

Since this probably isn't used in too many places throughout a code base, I think the tradeoff of adding extra characters for the sake of explicitness might be worth considering.

@styfle

This comment has been minimized.

Contributor

styfle commented Oct 19, 2018

On the topic of default exports...
Here are a list of reasons to avoid default exports (via @basarat)

https://basarat.gitbooks.io/typescript/docs/tips/defaultIsBad.html

I think most of the React community uses default exports however many, following the practices above, do not use default exports and prefer named exports.

@sebmarkbage

This comment has been minimized.

Collaborator

sebmarkbage commented Oct 19, 2018

The longer term vision includes making literally every component in the whole code base use this. So you can imagine that syntax overhead will be a big deal and familiarity counter balance any explicitness concerns.

@jquense

This comment has been minimized.

jquense commented Oct 19, 2018

The longer term vision includes making literally every component in the whole code base use this

I have a hard time imagining how this will play nicely with bundlers and efficient code splitting...at the moment that would create a new bundle per file in webpack no?

@sebmarkbage

This comment has been minimized.

Collaborator

sebmarkbage commented Oct 19, 2018

at the moment yes

// Annoying and confusing:
const Button = lazy(() => import('./Button').then(Button => Button.default));
// Named imports don't make this better:
const Button = lazy(() => import('./Button').then(Button => Button.Button));

This comment has been minimized.

@TrySound

TrySound Oct 19, 2018

Contributor

This is better

const Button = lazy(() => import('./Button').then(module => module.Button));

This comment has been minimized.

@gaearon

gaearon Oct 19, 2018

Member

The motivation for why it's not this way is explained just below. Did you get a chance to read it?

const Button = lazy(() => import('./Button').then(Button => Button.Button));
```
(Note this doesn't mean you're forced to use default exports for *all* your components. Even if you primarily use named exports, consider default exports to be "async entry points" into just the components you want to code split.)

This comment has been minimized.

@TrySound

TrySound Oct 19, 2018

Contributor

This adds inconsistency in modules style. The point of component code splitting to be invisible. You take any module, wrap it with lazy and it becomes loadable. Converting to default export is additional and wrong action. We don't need to define that module as asynchronous. Module should be only consumed as asynchronous in place.

This comment has been minimized.

@gaearon

gaearon Oct 19, 2018

Member

The motivation for this is explained just below.

```js
// Not a part of this RFC but plausible in the future
const Button = lazy(() => import('./components'), components => components.Button);

This comment has been minimized.

@TrySound

TrySound Oct 19, 2018

Contributor

This is nice, but we need something right now. Would be good to add at least an ugly example.

const Button = lazy(() =>
  import('./components').then(module => ({ default: module.Button }))
);

This comment has been minimized.

@sebmarkbage

sebmarkbage Oct 19, 2018

Collaborator

I think maybe we’ll try to warn against that pattern since it will break future work

This comment has been minimized.

@TrySound

TrySound Oct 19, 2018

Contributor

I don't get. How this will break future work? And how are you gonna forbid this? It's just a promise with object. We need a solution now. Using default exports for async components is awful workaround.

This comment has been minimized.

@gaearon

gaearon Oct 19, 2018

Member

Why do you feel so strongly against writing a single line of code that does the export?

This comment has been minimized.

@theKashey

theKashey Oct 19, 2018

Right now default exports are discouraged. There are a lot of reasons behind:

  • like allowSyntheticDefaultImports:false in typescript
  • like autoimports in IDE
  • like auto "naming" things

Why not to support basic "babel" interop, ie use .default if _esModules set, mimicking how "real"(babel/webpack) imports work?

I also prefer to have a default export on code splitting point, as some sort of an "interface", but it should not be the law.

This comment has been minimized.

@gaearon

gaearon Oct 19, 2018

Member

Babel interop has made the ESM ecosystem very messy and complex (even though it was instrumental in getting people to adopt ESM — perhaps too early). We don’t wait to add more tooling dependency on this behavior because it’s super hard to fix properly.

I also prefer to have a default export on code splitting point, as some sort of an "interface", but it should not be the law.

See #64 (comment).

@matt-casey

This comment has been minimized.

matt-casey commented Oct 19, 2018

The longer term vision includes making literally every component in the whole code base use this.

I might be able to see the utility in that if you have a massive team working on components in a distributed fashion, where what code is being rendered for any given user might be impossible to reason about.

For an average sized site, though, the extra latency of loading each component bundle individually would surely outweigh whatever gain you have from a faster initial page load.

Maybe in an HTTP/2 world?

@jquense

This comment has been minimized.

jquense commented Oct 19, 2018

practical named exports example btw Material UI exports a lot of components as named exports, if it makes sense maybe it's worth including the extension now? I also seethe value in limiting new API's initially

@salzhrani

This comment has been minimized.

salzhrani commented Oct 19, 2018

When rehydration is possible, there needs to be away to tell the client to eagerly load components that were loaded lazily on the server and/or prime the lazy components cache before the initial render.
maybe a provider like api that can be queried to provide such info, and given the injected data to prime the cache

@streamich

This comment has been minimized.

streamich commented Oct 19, 2018

Trying to find a component in .default seems totally arbitrary. It should just use the resolved value as a component. Or maybe check .default afterwards if the resolved value is not a component.

The promise that resolves to a component does not necessarily have to be a dynamic import().

Also, why force the community to use default exports for all components? Named exports are also a valid option, especially if you want to create a "bundle" of dynamically loaded components.

```js
// Annoying and confusing:
const Button = lazy(() => import('./Button').then(Button => Button.default));

This comment has been minimized.

@streamich

streamich Oct 19, 2018

Does not have to be written like that. Could be

const Button = lazy(async () => (await import('./Button')).default);

or even

const Button = lazyDefault(() => import('./Button'));

or

const Button = lazy(() => import('./Button'), 'default');

or

const Button = lazy(() => import('./Button').then(_ => _.default));

This comment has been minimized.

@milesj

milesj Oct 19, 2018

It most definitely needs to be a function that returns a promise, so the last 3 examples are invalid.

This comment has been minimized.

@streamich

This comment has been minimized.

@gabemeola

gabemeola Oct 19, 2018

it'll need the intermediate function so the dynamic import isn't immediately resolved.

This comment has been minimized.

@milesj

milesj Oct 19, 2018

Yup, without the function it's a side-effect.

@j-f1

This comment has been minimized.

j-f1 commented Oct 19, 2018

Can you explain how Suspense will be able to make use of the actual module record?

@GarethSmall

This comment has been minimized.

GarethSmall commented Oct 19, 2018

Here's an example: https://codesandbox.io/s/yx95qp06v

Suspense will provide a fallback option while it's waiting for the Promise to settle, once the the Promise resolves it will show the imported component.

@GarethSmall

This comment has been minimized.

GarethSmall commented Oct 19, 2018

@jquense

practical named exports example btw Material UI exports a lot of components as named exports, if it makes sense maybe it's worth including the extension now? I also seethe value in limiting new API's initially

Example of getting a Button from Material UI: https://codesandbox.io/s/kwzkor2p8v

@j-f1

This comment has been minimized.

j-f1 commented Oct 19, 2018

@GarethSmall Were you replying to me in your first comment? If so, how would the behavior be different if Suspense couldn’t access the module record, just the component?

@GarethSmall

This comment has been minimized.

GarethSmall commented Oct 19, 2018

@j-f1 Suspense doesn't access the module record, Suspense is expecting a component or at least the promise of a component.

@jquense

This comment has been minimized.

jquense commented Oct 19, 2018

@GarethSmall that behavior is changing, that some of the point of the RFC here

@GarethSmall

This comment has been minimized.

GarethSmall commented Oct 19, 2018

@j-f1 Do you mean the lazy function or the Suspense component?

@j-f1

This comment has been minimized.

j-f1 commented Oct 19, 2018

@GarethSmall I mean the React.lazy function, but the RFC says the module record will be useful to Suspense.

@GarethSmall

This comment has been minimized.

GarethSmall commented Oct 19, 2018

@j-f1 Ahh okay, I apologize I mis-spoke from a lack of understanding, so what I was doing was a bad practice.

Is this what you were talking about from the RFC?:

The second part of the question is why don't we still allow passing something without a .default property?

// Why don't we want to support this though?
const Button = lazy(async () => {
  const Components = await import('./components');
  // Resolve to named export:
  return components.Button;
});

We intentionally don't support this in the scope of this proposal. This gives React an opportunity to integrate more tightly with the module system on the server in the future. There are no proposed standards for this yet, but the new Suspense-capable React server renderer we'll soon be working on will benefit from having a chance to introspect the import() result directly (rather than just a component). For example, it could change a priority of a pending request for the Button.js module once it knows that a component of the Button type is going to be lazily rendered. We can't do this if we break the link between the module and the component. While this depends on future experimentation and standardization work, this design leaves more space for it. We can always remove this restriction later if it doesn't end up being beneficial. (That's also why the proposal doesn't support arbitrary Promises as component types.)

@j-f1

This comment has been minimized.

j-f1 commented Oct 19, 2018

Thanks for explaining; that makes a lot more sense.

@aweary

This comment has been minimized.

Member

aweary commented Oct 19, 2018

How does this interact with React.memo? Would this work?

const Button = React.memo(
  React.lazy(() =>
    import("third-party-button")
  )
);
@gaearon

This comment has been minimized.

Member

gaearon commented Oct 19, 2018

No. The argument to memo has to be a function component, which lazy isn't (it is a special type). We should error on this.

It works the other way around though. Which is consistent with what we say in memo RFC: it should be applied at the definition site. You can wrap in your own component at the call site if you really need this.

@aweary

This comment has been minimized.

Member

aweary commented Oct 19, 2018

The argument to memo has to be a function component, which lazy isn't (it is a special type). We should error on this.

Is this also true for React.forwardRef then?

(This is more of a question about memo, I'll move this discussion)

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 19, 2018

For an average sized site, though, the extra latency of loading each component bundle individually would surely outweigh whatever gain you have from a faster initial page load.

@matt-casey

I think you might be confusing the syntax (import is asynchronous) with a specific behavior (create a bundle per component). There's absolutely no reason why tools like webpack need to create a separate bundle per dynamic import. If they had access to usage statistics, they could instead be smart about bundling modules together in the most efficient way based on which are likely to be rendered together. Combined with a server rendering solution, modules could also be streamed in the order they're likely to be hydrated and used.

The "longer term" vision Sebastian alluded to is not about the current webpack behavior. But about allowing a better bundling solution.

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 19, 2018

Regarding named exports.

I hear where you’re all coming from. If you disagree with our recommendation to use default exports for split points, you can use a workaround described in #64 (comment). You could even your own lazyNamed helper that does this. Yes, it’s hacky and it might make it more difficult for you to benefit from the future work on the streaming rendering. But it seems like this isn’t a concern shared by many people on the thread. This makes sense since it applies to the future and not to today. And because details are very vague at this point (we’re just beginning to investigate this). We have to think about the future though because otherwise we’ll have to make breaking changes later.

This RFC has a limited surface. It's targeted at the main use case we want to start handling (default imports). There is a door open for supporting named imports in future RFCs, but it is out of scope of this one. This one is intentionally very limited. We expect to understand this problem space more within the next several months and then we can revisit that discussion with a better understanding.

I understand you want us to support your use case today. But we don’t have all the information yet, and if the solution is too broad, it’s very likely we’ll have to deprecate or make breaking changes to it in the next release. Nobody likes breaking changes. Therefore, for now we’d like to focus it on the parts we feel more confident in.

I hope this makes sense, even if you disagree.

Note: regardless of how lazy() works you will be able to build your own thing on top of Suspense that does something similar. So the decision to scope this RFC down won’t prevent you from lazy-loading named imports with your own helper.

@matt-casey

This comment has been minimized.

matt-casey commented Oct 19, 2018

The "longer term" vision Sebastian alluded to is not about the current webpack behavior. But about allowing a better bundling solution.

@gaearon
I think that makes sense, I was thinking very much in the context of today's webpack.

I can definitely imagine having everything lazy-loaded if the compiler were smart enough. It would be great to move that decision of where to code split out of the code-base itself and put it into the compiler's hands.

I realize now that in order to give the compiler the flexibility to do that, you'd have to do exactly what @sebmarkbage said and wrap literally every component in React.lazy.

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 19, 2018

When rehydration is possible, there needs to be away to tell the client to eagerly load components that were loaded lazily on the server and/or prime the lazy components cache before the initial render.

Yes. This is something we want to be able to do automatically, which is why we need the module information (rather than just the Promise to a component). Then the server could send this information to the client embedded in the small runtime.

@theKashey

This comment has been minimized.

theKashey commented Oct 19, 2018

It's not clear how server-side rendering works. To be more concrete - when component got imported.
It is not a question on a client side - all "userspace" loaders execute import function on Component render, and lazy will do the same. This is right and clear - import something only when you used it.

But on server side, it's very opinionated:

  • loadable-components waits for all components to be loaded, to do this library traverse react-tree by it's own
  • universal-component uses babel plugin to convert async imports to sync requires but still require some stuff only on component render
  • imported-components imports everything on HOC definition, so by the time server started - everything is already imported, and there are no actions to be made later. This also enabled some features, like per-connection mocking(here is an explanation why one need it), which could be very useful to make rendering more server-side friendly.

So - when imports will work on a server?

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 19, 2018

@theKashey

Supporting today's server renderer is out of scope of this proposal (as mentioned in the RFC).

In the future Suspense-capable server side renderer (which is fully asynchronous and can stream chunks while waiting for data) my mental model is that dynamic imports would be somehow rewritten to be synchronous, but the renderer would remember which modules got rendered, so that it can increase their priority as they're being sent to the client interleaved with data and HTML.

@shawnmitchell

This comment has been minimized.

shawnmitchell commented Oct 19, 2018

This is almost certainly outside the scope of this RFC but I wanted to see if there was any interest in lazy imports as a codebase security mechanism. What I'm thinking of is a paradigm where the code is stored in the cloud (Firebase Storage or the like) and streamed to and loaded by the client contingent on some separate backend user authentication.

@gaearon

This comment has been minimized.

Member

gaearon commented Oct 23, 2018

Going with this limited version (default imports only) for now. It's possible we'll add named imports later.

@gaearon gaearon merged commit d7dbd6e into master Oct 23, 2018

@bvaughn bvaughn deleted the gaearon-patch-2 branch Oct 23, 2018

@gaearon gaearon restored the gaearon-patch-2 branch Oct 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment