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

[WIP] feat: per module inlineDynamicImport #3299

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

manucorporat
Copy link
Contributor

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no (YET)

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

This PR allows to specify if a dynamically imported module should be inlined or not.

This allows rollup to:

  • Create a plugin that only inlines small bundles
  • Dynamic import of WebAssembly chunks without creating an extra JS

The WASM use case is interesting, since in order to load WASM eficiently, async is a must, a dynamic import covers the use case perfectly:

const {foo} = await import('./file.wasm');

The problem is that currently this import() will create two files (and meaning one extra round-trip):

  • The file.wasm.js chunk (the js code that loads the WASM):
const createAsyncWasm = (src) => {
  return WebAssembly.instantiateStreaming(fetch(src)).then(obj => obj.instance.exports);
};
const wasmPath = /*@__PURE__*/new URL('rust.rs-87ce7a05.wasm', import.meta.url).href;
const wasmExports = /*@__PURE__*/createAsyncWasm(wasmPath);
const then = wasmExports.then.bind(wasmExports);

and the file.wasm asset.

This new features allows to inline the JS, since the WASM is already loaded from a different asset

@manucorporat manucorporat changed the title feat: per module inlineDynamicImport [WIP] feat: per module inlineDynamicImport Dec 23, 2019
@lukastaegert lukastaegert self-requested a review December 28, 2019 09:11
@manucorporat
Copy link
Contributor Author

it will merge conflict with #3295
Waiting until it's merged

@codecov
Copy link

codecov bot commented Jan 10, 2020

Codecov Report

Merging #3299 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3299      +/-   ##
==========================================
+ Coverage   93.12%   93.14%   +0.01%     
==========================================
  Files         170      170              
  Lines        5967     5982      +15     
  Branches     1780     1785       +5     
==========================================
+ Hits         5557     5572      +15     
  Misses        219      219              
  Partials      191      191
Impacted Files Coverage Δ
src/utils/chunkColouring.ts 97.5% <100%> (+0.2%) ⬆️
src/Module.ts 97.34% <100%> (+0.03%) ⬆️
src/ModuleLoader.ts 98.51% <100%> (+0.03%) ⬆️
src/utils/transform.ts 95% <100%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5a3f16...de41198. Read the comment docs.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Hi, I added some tests in order to get a feeling for what this feature does. Unfortunately it turns out that this does completely prevent conditional execution of imports (besides the fact that it messes up execution order by executing code synchronously that should rather be executed async). I have no idea how this could be solved, i.e. what code Rollup could sensibly generate to address this. If you think the feature is still useful as it is, then we can definitely discuss this.

@@ -19,6 +19,11 @@ export function assignChunkColouringHashes(
Uint8ArrayXor(module.entryPointsHash, currentEntryHash);
}

for (const { resolution } of module.dynamicImports) {
if (resolution instanceof Module && resolution.inlineDynamicImport) {
module.dependencies.push(resolution);
Copy link
Member

Choose a reason for hiding this comment

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

I do not think this is how we should do it. The reason is that module.dependencies is responsible for which imports the chunk has that this module ends up in. This means that if the target of the inlined dynamic import ends up in a different chunk than the import expression, this chunk will generate both the dynamic import as well as a static one. I added a test that show-cases this.

@@ -0,0 +1,5 @@
import './generated-inlined.js';

import('./generated-inlined.js').then(console.log);
Copy link
Member

Choose a reason for hiding this comment

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

Notice how here, we have both a static as well as a dynamic import to the chunk containing the shared dynamic import target. The reason is because the dynamic import is added to the modules dependencies.

var inlined = /*#__PURE__*/Object.freeze({
__proto__: null,
value: value
});
Copy link
Member

Choose a reason for hiding this comment

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

This is the single biggest problem I encountered with the current implementation of this feature: The dynamic imports are no longer conditionally executed, they are immediately and synchronously executed. See this example: If globalThis.unknown is false, the namespace is not read, but the side-effect log statement of the dynamic import is still triggered. I am not sure this feature is valuable if this does not work. However I have no idea how to actually make it work. If we start wrapping code in conditionally executed function closures, then there would still be the problem if a dynamically imported module that is inlined has itself a static dependency that ends up in yet another chunk—how should this be handled? There are no conditional static imports.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, it could technically be done by first dynamically importing the static dependencies that are in other chunks and THEN executing the conditionally imported code. But it would become quite a bit bigger than it is now because then we need to think about the whole chunk rendering process, not only the colouring algorithm. If we want to go there, I would suggest we should collect a few scenarios with expected sample code first before thinking about the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants