[WIP]feat(pack): library runtime code compitable with Array #2467
Conversation
Summary of ChangesHello @fireairforce, 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 significantly refactors the core JavaScript runtime within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
fireairforce
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the module factory registration to use a more compact array format, which is a good optimization for bundle size. The changes are extensive, touching both Rust code generation and the TypeScript runtime. However, I've identified several critical issues. The registerChunk function in build-base.ts appears to have a bug where it doesn't register modules when runtime parameters are present. The TypeScript type for ChunkRegistration is invalid. There's also a regression where error checks for cached failed modules have been removed, which could lead to silent failures. Lastly, the snapshot test for the library build runtime seems to be broken or outdated, as it doesn't reflect the new array format for module factories, indicating the changes might not be properly tested.
| if (registration.length === 3) { | ||
| runtimeParams = registration[2] as RuntimeParams; | ||
| } else { | ||
| runtimeParams = undefined; | ||
| installCompressedModuleFactories( | ||
| registration as CompressedModuleFactories, | ||
| /* offset= */ 1, | ||
| moduleFactories, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The logic in registerChunk is flawed. When registration.length === 3, it correctly extracts runtimeParams but fails to process registration[1], which contains the module factories. This will prevent any modules from being registered for chunks that have runtime parameters.
if (registration.length === 3) {
runtimeParams = registration[2] as RuntimeParams;
installCompressedModuleFactories(
registration[1] as CompressedModuleFactories,
/* offset= */ 0,
moduleFactories,
);
} else {
runtimeParams = undefined;
installCompressedModuleFactories(
registration as CompressedModuleFactories,
/* offset= */ 1,
moduleFactories,
);
}| function registerChunk(registration) { | ||
| const chunkPath = getPathFromScript(registration[0]); | ||
| let runtimeParams; | ||
| // When bootstrapping we are passed a single runtimeParams object so we can distinguish purely based on length | ||
| if (registration.length === 2) { | ||
| runtimeParams = registration[1]; | ||
| } else { | ||
| runtimeParams = undefined; | ||
| installCompressedModuleFactories(registration, /* offset= */ 1, moduleFactories); | ||
| } | ||
| return BACKEND.registerChunk(chunkPath, runtimeParams); | ||
| } |
There was a problem hiding this comment.
This new implementation of registerChunk seems incorrect. It checks registration.length === 2 to identify bootstrap params, but the data passed at the end of this file is an array of length 3 (["main.js", {modules...}, {params...}]). This will cause the else branch to be taken, which calls installCompressedModuleFactories. This function expects a flat array of modules, but it will receive ["main.js", {modules...}, {params...}], which will cause a "malformed chunk format" error at runtime.
Furthermore, the module data at the end of the file is still in the old object format, not the new array format that chunk.rs is supposed to generate. This indicates the snapshot is not correctly updated to test the changes in this PR.
| type ChunkRegistration = [ | ||
| chunkPath: ChunkScript, | ||
| chunkModules: CompressedModuleFactories, | ||
| params: RuntimeParams | undefined, | ||
| ...(CompressedModuleFactories | [RuntimeParams]), | ||
| ]; |
There was a problem hiding this comment.
The type definition for ChunkRegistration appears to be invalid TypeScript. The spread operator ... cannot be applied to a union of an array type and a tuple type like CompressedModuleFactories | [RuntimeParams]. A simpler tuple type would be more appropriate and correct, reflecting that params is optional.
| type ChunkRegistration = [ | |
| chunkPath: ChunkScript, | |
| chunkModules: CompressedModuleFactories, | |
| params: RuntimeParams | undefined, | |
| ...(CompressedModuleFactories | [RuntimeParams]), | |
| ]; | |
| type ChunkRegistration = [ | |
| chunkPath: ChunkScript, | |
| chunkModules: CompressedModuleFactories, | |
| params?: RuntimeParams, | |
| ]; |
| if (module.error) throw module.error; | ||
|
|
||
| // any ES module has to have `module.namespaceObject` defined. | ||
| if (module.namespaceObject) return module.namespaceObject; |
There was a problem hiding this comment.
The removal of if (module.error) throw module.error; could lead to suppressed errors. If a module fails to instantiate, it is cached with an error property. Subsequent imports of this module will retrieve it from the cache but will no longer re-throw the error, potentially leading to silent failures or unexpected behavior. It's recommended to re-introduce this check.
| if (module.namespaceObject) return module.namespaceObject; | |
| if (module.error) throw module.error; |
| if (module.error) throw module.error; | ||
| return module.exports; | ||
| return getOrInstantiateModuleFromParent(id, this.m).exports; | ||
| } |
There was a problem hiding this comment.
Similar to esmImport, removing the error check if (module.error) throw module.error; can lead to suppressed errors for failed module instantiations that are retrieved from the cache.
const module = getOrInstantiateModuleFromParent(id, this.m);
if (module.error) throw module.error;
return module.exports;| let code_key = item_code | ||
| .source_code() | ||
| .to_str() | ||
| .map_err(|e| anyhow::anyhow!("Failed to convert code to string: {}", e))? | ||
| .to_string(); |
There was a problem hiding this comment.
Using the full source code of a module as a HashMap key can be inefficient, especially if the code is large. This involves string conversion and hashing the entire content on each insertion. Consider using a more efficient key, such as a hash of the code content, if performance at build time becomes a concern.
xusd320
force-pushed
the
next
branch
2 times, most recently
from
f77808c to
567e5ae
Compare
Summary
compitable with: vercel/next.js#81877 and vercel/next.js#82760
Test Plan