feat(turbopack-node): emit loader assets that can be run with systemjs #75
feat(turbopack-node): emit loader assets that can be run with systemjs #75
Conversation
Summary of ChangesHello @xusd320, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant architectural changes to Turbopack's Node.js build system, primarily to support WebAssembly (Wasm) compilation targets. It refactors the runtime and loader execution mechanisms to operate efficiently within Wasm environments, such as Edge Workers. This involves creating a dedicated Wasm-compatible Node.js runtime, enabling Webpack loaders to run in isolated Web Workers, and adapting the build environment configuration to reflect these new execution contexts. The changes ensure that Turbopack can effectively process and emit assets for platforms that leverage Wasm for server-side or edge computing. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for running loader assets with SystemJS in a WASM environment. This is achieved by adding a new Node.js runtime variant for WASM (runtime.utoowasm.ts) and a web worker-based implementation for running webpack loaders. The changes involve conditional compilation in Rust files to switch between the standard Node.js environment and the new worker environment for WASM targets.
I've found a few critical issues that need to be addressed:
- The new WASM runtime is missing some essential script parts, which will likely cause it to fail.
- The
fs.readFileshim in the webpack loader worker is implemented incorrectly, which will break loaders that read files. - There's a mismatch in a function call signature in the new runtime.
Details are in the comments below.
| let mut code = CodeBuilder::default(); | ||
| code.push_code(&*shared_runtime_utils_code.await?); | ||
| code.push_code(&*shared_base_external_utils_code.await?); | ||
| code.push_code(&*runtime_code.await?); |
There was a problem hiding this comment.
The wasm-targeted runtime is missing necessary script parts. The runtime.utoowasm.ts file references node-externals-utils.ts and node-wasm-utils.ts, but unlike the non-wasm version of get_nodejs_runtime_code, this function doesn't embed them. This will likely lead to runtime errors. You should embed these files here, similar to the non-wasm implementation.
| async () => { | ||
| await self.workerData.readFile(p, optionsOrCb).then(maybeCb) | ||
| }, |
There was a problem hiding this comment.
The fs.readFile shim is incorrectly adapting the promise-based self.workerData.readFile to the callback-style API expected by loader-runner.
optionsOrCb(which can be a callback) is passed as theencodingtoself.workerData.readFile.- The
.then(maybeCb)logic is incorrect.fs.readFilecallbacks expect(error, data), but.thenprovides onlydataon success.
This will cause loaders that read files to fail.
() => {
const callback = typeof optionsOrCb === 'function' ? optionsOrCb : maybeCb;
const encoding = typeof optionsOrCb === 'object' && optionsOrCb !== null ? optionsOrCb.encoding : undefined;
if (encoding && encoding !== 'utf8') {
callback(new Error(`Unsupported encoding for readFile: ${encoding}. Only 'utf8' is supported.`));
return;
}
self.workerData.readFile(p, encoding).then(
(data) => callback(null, encoding ? data : Buffer.from(data, 'utf8')),
(err) => callback(err)
);
},| ) | ||
| // NOTE(alexkirsz) This can fail when the module encounters a runtime error. | ||
| try { | ||
| moduleFactory(context, module, exports) |
There was a problem hiding this comment.
The call to moduleFactory does not match its type definition. The type ModuleFactory is defined as (this: Module['exports'], context: TurbopackNodeBuildContext) => unknown, which expects this to be module.exports and to receive only the context argument. The current call does not set this and passes extra arguments. This will likely lead to runtime errors inside the module.
| moduleFactory(context, module, exports) | |
| moduleFactory.call(exports, context); |
829f0a9 to
fdaf929
Compare
fdaf929 to
634c96b
Compare
13dc217 to
7adc2b9
Compare
As titled.