refact(wasm): use helper module instead of banner #607
Conversation
|
Thanks for the PR. You've checked the box that tests are included but haven't provided any tests in the PR. Please add tests and we'll review. |
| @@ -0,0 +1,55 @@ | |||
| export const HELPERS_ID = '\0wasmHelpers.js'; | |||
There was a problem hiding this comment.
What does this null byte prefix do?
There was a problem hiding this comment.
I am not 100 percent sure where to find the documentation, but "\0" seems to be a convention when writing rollup plugin to indicate the module comes from a plugin. and if it should not be taken by this plugin, resolveId returns null, otherwise returns the current resolveId
There was a problem hiding this comment.
It indicates a "virtual module" in the Rollup ecosystem.
There was a problem hiding this comment.
More precisely it means: Other plugins, please ignore this module and please do not apply any transformations etc. https://rollupjs.org/guide/en/#conventions
Hi @shellscape. No more tests are added in this PR. This is a refactoring for the existing functionality, so it might be good enough to pass all existing tests. |
|
Sorry, but we cannot accept pull requests for fixes or features without accompanying tests. You are refactoring some code, but what your refactor attempts to accomplish is a fix. You must write new tests that demonstrate the conditions your changes resolve before we can accept the PR. This requirement exists to protect against future regression. |
@shellscape I totally agree to prevent future regression by adding new tests, but kind of confusing about how to add new tests for this refactor. Since IMO the existing tests cover the functionality of the current codebase, and this PR does not add/remove more functionality...(just moving the code in Could you kindly help to figure out the direction more clear? |
|
Sure. You have a clear before/after result. Previously the bundle would contain multiple copies of the helper. With your changes it should contain only one. That's the test. |
chengcyber
force-pushed
the
refact-wasm
branch
from
2cb42a8 to
2f33743
Compare
|
@shellscape A test for injecting helper is added, using a custom detect plugin to check whether the module contains inject import. PTAL |
| t.true(code.includes(injectImport)); | ||
| } | ||
| if (id.endsWith('foo.js')) { | ||
| t.true(!code.includes(injectImport)); |
There was a problem hiding this comment.
| t.true(!code.includes(injectImport)); | |
| t.false(code.includes(injectImport)); |
|
@bminixhofer since you recently contributed here, please have a look at this change. |
lukastaegert
left a comment
lukastaegert
left a comment
There was a problem hiding this comment.
Did not test it in detail but looks good to me!
| @@ -0,0 +1,55 @@ | |||
| export const HELPERS_ID = '\0wasmHelpers.js'; | |||
There was a problem hiding this comment.
More precisely it means: Other plugins, please ignore this module and please do not apply any transformations etc. https://rollupjs.org/guide/en/#conventions
|
Looks good to me as well! |
|
|
5 participants
Rollup Plugin Name:
@rollup/plugin-wasmThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Description
For now,
wasmplugin is usingbannerAPI to inject helper function_loadWasmModule, but it will inject this helper function in every file, even those files is unrelated to wasm things.After this PR merged, it uses a helper module to inject
_loadWasmModule, hopes to help removing the redundant code