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

[Flight] Wire up bundler configs #18334

Merged
merged 1 commit into from
Mar 18, 2020

Conversation

sebmarkbage
Copy link
Collaborator

This allows different Flight server and clients to have different configs depending on bundler to serialize and resolve modules.

I'll use these in a follow up for Blocks.

@sebmarkbage sebmarkbage requested a review from gaearon March 18, 2020 02:28
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 18, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 18, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit def1f72:

Sandbox Source
quizzical-meitner-g1mti Configuration

@sizebot
Copy link

sizebot commented Mar 18, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against def1f72

@sizebot
Copy link

sizebot commented Mar 18, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against def1f72

if (entry) {
return entry.exports.default;
}
// Ideally Webpack would let us inspect that all chunks have loaded and
Copy link
Collaborator

@gaearon gaearon Mar 18, 2020

Choose a reason for hiding this comment

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

I chatted to @sokra about this and he says they can make it return the same Promise every time so we can put it in a WeakMap. Would this help us? My understanding is that this wouldn't be sufficient. If we just needed to track chunk completion status we could do it ourselves:

__webpack_chunk_load__(chunk).then(() => { status[chunk] = true })

But that doesn't solve the problem of checking whether a particular chunk has already loaded on its own (either as the initial script on the page or as a part of some other request outside of Flight). I think that's what you want to check? In that case, I don't see a different way than exposing installedChunks. Maybe we can write a plugin that puts it on a global.

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Mar 18, 2020

Choose a reason for hiding this comment

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

Avoiding every possible async point is not a goal here. Just to minimize them. As discussed, I think the key here is to avoid the waterfall of requests that happens when each React component asks for them one by one. However, in this new approach we have all the info about all the chunks we're at high likelihood going to need soon without executing the React components. That's kind of the point because we should eagerly prefetch them regardless.

So I'm going to refactor this to eagerly load all the chunks and then keep track on our side whether it has already loaded. That way even if one needs to suspend, in that gap, we'll be able to collect a lot of newly resolved chunks at once and then continue.

That way we don't need to rely on the synchronous resolution in Webpack (or ESM) and it makes more sense anyway since there's a difference between a preloaded chunk and one that is inserted as a script tag (more aggressive).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The key difference between this and the dynamic imports API is that loading the chunks doesn't immediately execute the module bodies. If we did, then executing these eagerly wouldn't be palatable.

So the deep Webpack integration is still useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that chunk loading can be more than just loading javascript:

  • adding stylesheets when using the mini-css-extract-webpack-plugin
  • compiling wasm when using the sync wasm approach (deprecated in webpack 5)
  • prefetching and preloading more chunks
  • custom plugins would add additional logic here (e. g. I wrote a localization plugin which fetches localization data during chunk loading).

So direct installedChunks access would break when using more than JS.

This allows different flight server and clients to have different configs
depending on bundler to serialize and resolve modules.
// If they're still pending they're a thenable. This map also exists
// in Webpack but unfortunately it's not exposed so we have to
// replicate it in user space. null means that it has already loaded.
const chunkCache: Map<string, null | Thenable> = new Map();
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Mar 18, 2020

Choose a reason for hiding this comment

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

@sokra In this approach, I'll always call __webpack_chunk_load__ eagerly so there's always an opportunity for it to kick off async requests as a result. Always getting the same Promise back would be helpful since I wouldn't need to cache it. However, I also need to know if they've all been resolved so I know it's safe to call require.

This means I have to replicate this information in a user space map anyway.

It would be nice if there was a way to call __webpack_chunk_load__, so that there's something that spawns async work to kick off but synchronously know if it's already safe to call __webpack_require__. It's fine that we're forced to call __webpack_chunk_load__ and even that it's always async at the first call, but if we have already done so in the past, it would be nice to query the webpack cache rather than our own.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const chunkCache: Map<string, null | Thenable> = new Map();
const chunkCache: Map<string | number, null | Thenable> = new Map();

I consider adding an extra argument to __webpack_chunk_load__ which make it return null instead of a resolved Promise when the chunk is already loaded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be great! Thank you.

@sebmarkbage sebmarkbage merged commit 8206b4b into facebook:master Mar 18, 2020
}
let chunks = moduleData.chunks;
let anyRemainingThenable = null;
for (let i = 0; i < chunks.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code snippet doesn't work for more than 1 chunks, as anyRemainingThenable is overwritten in each iteration.

You probably want to use Promise.all here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will be recalled each time a new chunk resolves which will then get the next one and so on. However in practice it most likely won’t get as far as even retrying the second time until everything has been resolved.

So in practice we get one promise, then wait for it, schedule work to try again, in the meantime all of them resolve and then we try again just to confirm they all resolved. If not we’ll do the whole thing again.

// can synchronously require the module now. A thenable is returned
// that when resolved, means we can try again.
export function preloadModule(moduleData: ModuleMetaData): null | Thenable {
let moduleEntry = require.cache[moduleData.id];
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically using require.cache and ESM is kind of illegal. It works in webpack in "automatic" JS mode (kind of mixed CJS and ESM), but require.cache is not available in strict ESM mode. At least it works for webpack <= 5.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have you considered exposing a Webpack specific helper to do something similar to this?

In this case I could just get rid of it since it’s just an optimization to avoid having to check many maps.

It might be nice to know for scheduling purposes though. E.g. we try to be very particular about when we call __webpack_require__ since that can kick off a lot of synchronous work. So it’s nice to be able to know if the call will be free or hundreds of ms.

}

export function requireModule<T>(moduleData: ModuleMetaData): T {
return __webpack_require__(moduleData.id).default;
Copy link
Contributor

Choose a reason for hiding this comment

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

In production mode webpack may mangles exports names if certain conditions are met. This means .default might be .n or something else.

It depends on how the module is used and how you get the id. You could opt-out from these optimization, but the better idea is probably to add the mangled export name to the ModuleMetaData. I guess you extract this meta data with a webpack plugin. Here you can also access the final name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Yea that sounds like a good idea.

Comment on lines +11 to +12
id: string,
chunks: Array<string>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
id: string,
chunks: Array<string>,
id: string | number,
chunks: Array<string | number>,

// If they're still pending they're a thenable. This map also exists
// in Webpack but unfortunately it's not exposed so we have to
// replicate it in user space. null means that it has already loaded.
const chunkCache: Map<string, null | Thenable> = new Map();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const chunkCache: Map<string, null | Thenable> = new Map();
const chunkCache: Map<string | number, null | Thenable> = new Map();

I consider adding an extra argument to __webpack_chunk_load__ which make it return null instead of a resolved Promise when the chunk is already loaded.

sebmarkbage added a commit to sebmarkbage/react that referenced this pull request Mar 20, 2020
Follow ups from facebook#18334

I also introduced the concept of a module reference on the client too.
We don't need this for webpack so that gets compiled out but we need it
for www. Similarly I also need a difference between preload and load.
sebmarkbage added a commit that referenced this pull request Mar 20, 2020
Follow ups from #18334

I also introduced the concept of a module reference on the client too.
We don't need this for webpack so that gets compiled out but we need it
for www. Similarly I also need a difference between preload and load.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants