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] Basic scan of the file system to find Client modules #20383

Merged
merged 2 commits into from
Dec 7, 2020
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
9 changes: 8 additions & 1 deletion fixtures/flight/config/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -664,7 +664,14 @@ module.exports = function(webpackEnv) {
formatter: isEnvProduction ? typescriptFormatter : undefined,
}),
// Fork Start
new ReactFlightWebpackPlugin({isServer: false}),
new ReactFlightWebpackPlugin({
isServer: false,
clientReferences: {
directory: './src/',
recursive: true,
include: /\.client\.js$/,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have a plan for node_modules? Am I supposed to exclude them here? Do they follow a different convention?

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Dec 5, 2020

Choose a reason for hiding this comment

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

You’re supposed to include them. That’s why this is such a bad idea and it’s better to compile server first and use the reachable ones as a list here.

But there are tradeoffs and both approaches are valid.

This affords much more parallelism which is why we do something similar at FB. And we don’t have thousands of unused files in our graph.

Copy link
Collaborator

@gaearon gaearon Dec 5, 2020

Choose a reason for hiding this comment

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

I think I'm still fuzzy on how you distribute Server+Client over npm. E.g. the default on npm today is Client. So if I import react-something from a Server one then I'd actually expect this to behave like a .client entry point because the package author doesn't know about the distinction.

If we were to start from scratch then defaulting to Server makes sense. But I don't know how this would work with the existing ecosystem?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You will get a static error on the server because you’re trying to import an export from React that doesn’t exist like useState. At least with real ESM. (We might want to fast track ESM for that reason.)

Existing things that just work as hybrids continue to work. Others will error on the server and you have to wrap it in a .client file. Ideally the publisher would’ve already done that but if not, you have to add a local wrapper to fix the error.

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 I think it’s the resolved file that needs to be .client.js. So you can add an “exports” alias so the import is just plain.

That’s no different than a hybrid wrapper that references a client.

Copy link
Collaborator

Choose a reason for hiding this comment

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

makes sense

},
}),
// Fork End
].filter(Boolean),
// Some libraries import Node modules but don't use them in the browser.
Expand Down
4 changes: 0 additions & 4 deletions fixtures/flight/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,3 @@ ReactDOM.render(
</Suspense>,
document.getElementById('root')
);

// Create entry points for Client Components.
// TODO: Webpack plugin should do this.
require.context('./', true, /\.client\.js$/, 'lazy');
1 change: 1 addition & 0 deletions packages/react-transport-dom-webpack/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
},
"dependencies": {
"acorn": "^6.2.1",
"neo-async": "^2.6.1",
"loose-envify": "^1.1.0",
"object-assign": "^4.1.1"
},
Expand Down
234 changes: 231 additions & 3 deletions packages/react-transport-dom-webpack/src/ReactFlightWebpackPlugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,169 @@
*/

import {mkdirSync, writeFileSync} from 'fs';
import {dirname, resolve} from 'path';
import {dirname, resolve, join} from 'path';
import {pathToFileURL} from 'url';

import asyncLib from 'neo-async';

import ModuleDependency from 'webpack/lib/dependencies/ModuleDependency';
import NullDependency from 'webpack/lib/dependencies/NullDependency';
import AsyncDependenciesBlock from 'webpack/lib/AsyncDependenciesBlock';
import Template from 'webpack/lib/Template';

class ClientReferenceDependency extends ModuleDependency {
constructor(request) {
super(request);
}

get type() {
return 'client-reference';
}
}

// This is the module that will be used to anchor all client references to.
// I.e. it will have all the client files as async deps from this point on.
// We use the Flight client implementation because you can't get to these
// without the client runtime so it's the first time in the loading sequence
// you might want them.
const clientFileName = require.resolve('../');

type ClientReferenceSearchPath = {
directory: string,
recursive?: boolean,
include: RegExp,
exclude?: RegExp,
};

type ClientReferencePath = string | ClientReferenceSearchPath;

type Options = {
isServer: boolean,
clientReferences?: ClientReferencePath | $ReadOnlyArray<ClientReferencePath>,
chunkName?: string,
};

const PLUGIN_NAME = 'React Transport Plugin';

export default class ReactFlightWebpackPlugin {
constructor(options: {isServer: boolean}) {}
clientReferences: $ReadOnlyArray<ClientReferencePath>;
chunkName: string;
constructor(options: Options) {
if (!options || typeof options.isServer !== 'boolean') {
throw new Error(
PLUGIN_NAME + ': You must specify the isServer option as a boolean.',
);
}
if (options.isServer) {
throw new Error('TODO: Implement the server compiler.');
}
if (!options.clientReferences) {
this.clientReferences = [
{
directory: '.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this? Project cwd?

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Dec 5, 2020

Choose a reason for hiding this comment

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

It’s relative to Webpack’s virtual file system. So it’s whatever that is. Nothing touches a real file system directly.

recursive: true,
include: /\.client\.(js|ts|jsx|tsx)$/,
},
];
} else if (
typeof options.clientReferences === 'string' ||
!Array.isArray(options.clientReferences)
) {
this.clientReferences = [(options.clientReferences: $FlowFixMe)];
} else {
this.clientReferences = options.clientReferences;
}
if (typeof options.chunkName === 'string') {
this.chunkName = options.chunkName;
if (!/\[(index|request)\]/.test(this.chunkName)) {
this.chunkName += '[index]';
}
} else {
this.chunkName = 'client[index]';
}
}

apply(compiler: any) {
compiler.hooks.emit.tap('React Transport Plugin', compilation => {
const run = (params, callback) => {
// First we need to find all client files on the file system. We do this early so
// that we have them synchronously available later when we need them. This might
// not be needed anymore since we no longer need to compile the module itself in
// a special way. So it's probably better to do this lazily and in parallel with
// other compilation.
const contextResolver = compiler.resolverFactory.get('context', {});
this.resolveAllClientFiles(
compiler.context,
contextResolver,
compiler.inputFileSystem,
compiler.createContextModuleFactory(),
(err, resolvedClientReferences) => {
if (err) {
callback(err);
return;
}
compiler.hooks.compilation.tap(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it safe to tap asynchronously like this in a callback? Is there a chance that we might miss events while resolveAllClientFiles is still collecting them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe this is a blocking phase so Webpack doesn’t proceed until we’ve invoked the callback.

That’s also why this approach is suboptimal since it doesn’t allow for other parallelism in the meantime.

That was a limitation in the previous approach that needed to change/split the files.

We could put this elsewhere but it’s not broken.

PLUGIN_NAME,
(compilation, {normalModuleFactory}) => {
compilation.dependencyFactories.set(
ClientReferenceDependency,
normalModuleFactory,
);
compilation.dependencyTemplates.set(
ClientReferenceDependency,
new NullDependency.Template(),
);

compilation.hooks.buildModule.tap(PLUGIN_NAME, module => {
// We need to add all client references as dependency of something in the graph so
// Webpack knows which entries need to know about the relevant chunks and include the
// map in their runtime. The things that actually resolves the dependency is the Flight
// client runtime. So we add them as a dependency of the Flight client runtime.
// Anything that imports the runtime will be made aware of these chunks.
// TODO: Warn if we don't find this file anywhere in the compilation.
if (module.resource !== clientFileName) {
return;
}
if (resolvedClientReferences) {
for (let i = 0; i < resolvedClientReferences.length; i++) {
const dep = resolvedClientReferences[i];
const chunkName = this.chunkName
.replace(/\[index\]/g, '' + i)
.replace(
/\[request\]/g,
Template.toPath(dep.userRequest),
);

const block = new AsyncDependenciesBlock(
{
name: chunkName,
},
module,
null,
dep.require,
);
block.addDependency(dep);
module.addBlock(block);
}
}
});
},
);

callback();
},
);
};

compiler.hooks.run.tapAsync(PLUGIN_NAME, run);
compiler.hooks.watchRun.tapAsync(PLUGIN_NAME, run);

compiler.hooks.emit.tap(PLUGIN_NAME, compilation => {
const json = {};
compilation.chunks.forEach(chunk => {
chunk.getModules().forEach(mod => {
// TOOD: Hook into deps instead of the target module.
// That way we know by the type of dep whether to include.
// It also resolves conflicts when the same module is in multiple chunks.
if (!/\.client\.js$/.test(mod.resource)) {
return;
}
Expand All @@ -42,7 +194,83 @@ export default class ReactFlightWebpackPlugin {
'react-transport-manifest.json',
);
mkdirSync(dirname(filename), {recursive: true});
// TODO: Use webpack's emit API and read from the devserver.
writeFileSync(filename, output);
});
}

// This attempts to replicate the dynamic file path resolution used for other wildcard
// resolution in Webpack is using.
resolveAllClientFiles(
context: string,
contextResolver: any,
fs: any,
contextModuleFactory: any,
callback: (
err: null | Error,
result?: $ReadOnlyArray<ClientReferenceDependency>,
) => void,
) {
asyncLib.map(
this.clientReferences,
(
clientReferencePath: string | ClientReferenceSearchPath,
cb: (
err: null | Error,
result?: $ReadOnlyArray<ClientReferenceDependency>,
) => void,
): void => {
if (typeof clientReferencePath === 'string') {
cb(null, [new ClientReferenceDependency(clientReferencePath)]);
return;
}
const clientReferenceSearch: ClientReferenceSearchPath = clientReferencePath;
contextResolver.resolve(
{},
context,
clientReferencePath.directory,
{},
(err, resolvedDirectory) => {
if (err) return cb(err);
const options = {
resource: resolvedDirectory,
resourceQuery: '',
recursive:
clientReferenceSearch.recursive === undefined
? true
: clientReferenceSearch.recursive,
regExp: clientReferenceSearch.include,
include: undefined,
exclude: clientReferenceSearch.exclude,
};
contextModuleFactory.resolveDependencies(
fs,
options,
(err2: null | Error, deps: Array<ModuleDependency>) => {
if (err2) return cb(err2);
const clientRefDeps = deps.map(dep => {
const request = join(resolvedDirectory, dep.request);
const clientRefDep = new ClientReferenceDependency(request);
clientRefDep.userRequest = dep.userRequest;
return clientRefDep;
});
cb(null, clientRefDeps);
},
);
},
);
},
(
err: null | Error,
result: $ReadOnlyArray<$ReadOnlyArray<ClientReferenceDependency>>,
): void => {
if (err) return callback(err);
const flat = [];
for (let i = 0; i < result.length; i++) {
flat.push.apply(flat, result[i]);
}
callback(null, flat);
},
);
}
}
2 changes: 1 addition & 1 deletion scripts/rollup/bundles.js
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ const bundles = [
moduleType: RENDERER_UTILS,
entry: 'react-transport-dom-webpack/plugin',
global: 'ReactFlightWebpackPlugin',
externals: ['fs', 'path', 'url'],
externals: ['fs', 'path', 'url', 'neo-async'],
},

/******* React Transport DOM Webpack Node.js Loader *******/
Expand Down