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

Enable specifying implicit dependencies when emitting chunks #3606

Merged
merged 16 commits into from Jun 3, 2020

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented May 29, 2020

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

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

Breaking Changes?

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

List any relevant issue numbers:
Resolves #3574

Description

This will enable chunk emitters to specify that an otherwise independent chunk will only be loaded if at least one of a list of modules has been loaded before, allowing Rollup to create fewer chunks. The logic will be the same as if the chunk was only reachable via dynamic import from any of the specified module ids. A good use-case could be when parsing an HTML file for scripts as entry points—subsequent scripts can rely on previous scripts having loaded and can import shared dependencies directly from the chunks of the previous scripts.

The API of this.emitFile will be extended to accept

interface EmittedChunk {
	fileName?: string;
	id: string;
	// This is the new property; if this is a non-empty array of module ids,
	// the new logic will kick in
	implicitlyLoadedAfterOneOf?: string[];
	importer?: string;
	name?: string;
	preserveSignature?: PreserveEntrySignaturesOption;
	type: 'chunk';
}

If there are one or more module ids in implicitlyLoadedAfterOneOf, then this chunk will assume that at least one of the chunks containing those modules is already in memory, basically exactly as if each of those modules contained a dynamic import of the newly emitted chunk.

This will be reflected in this.getModuleInfo

interface ModuleInfo {
	dynamicallyImportedIds: string[];
	dynamicImporters: string[];
	hasModuleSideEffects: boolean;
	id: string;
	// New properties to find implicit dependencies and dependants
	implicitlyLoadedAfterOneOf: string[];
	implicitlyLoadedBefore: string[];
	importedIds: string[];
	importers: string[];
	isEntry: boolean;
	isExternal: boolean;
}

as well as the bundle

interface PreRenderedChunk {
	code?: string;
	dynamicImports: string[];
	exports: string[];
	facadeModuleId: string | null;
	fileName?: string;
	// new property
	implicitlyLoadedBefore: string[];
	imports: string[];
	isDynamicEntry: boolean;
	isEntry: boolean;
	// new property
	isImplicitEntry: boolean;
	map?: SourceMap;
	modules: {
		[id: string]: RenderedModule;
	};
	name: string;
	type: 'chunk';
}

Things to do and clarify:

  • How do we handle the case where one of the implicitlyLoadedAfterOneOf modules is not part of the output graph, either because it is not found, it is empty, or it is only reachable via a tree-shaken dynamic import? Do we want to throw in all cases, or ignore those modules up to the point where our module is promoted to a regular entry point?
  • Chunks with implicitlyLoadedAfterOneOf should respect both the user-defined preserveEntrySignatures option as well as the preserveSignature emission option
  • Test: Emitted chunk is only loaded after an empty entry module
  • Test: Emitted chunk is only loaded after an empty non-entry module
  • Test: A chunk is emitted with several implicitlyLoadedAfterOneOf entries
  • Test: A chunk is emitted several times with different implicitlyLoadedAfterOneOf entries. In that case, we merge those entries. Could be combined with the previous test.
  • Test: A chunk is loaded after a dynamically imported module where the dynamic import is not included.
  • Test: A chunk is loaded after a module that is not part of the graph.
  • Test: A chunk is loaded after a module that is not found.
  • Test: A chunk is loaded after a module that is external.
  • Test: A chunk is loaded after a module in the same chunk.
  • Test: A chunk is emitted several times, once without implicit dependants
  • Test: An entry chunk is emitted with implicit dependants

@rollup-bot
Copy link
Collaborator

rollup-bot commented May 29, 2020

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#gh-3574-implicit-dependencies

or load it into the REPL:
https://rollupjs.org/repl/?circleci=11614

@lukastaegert
Copy link
Member Author

At the moment, the new property is implicitDependants and a chunk is an implicitDependency if it is only loaded AFTER the chunk it is a dependency of. This is taken from the terms used for dynamic imports but I find it very confusing the more I think about it.

Maybe when emitting, we call the property implicitlyLoadedAfterOneOf and the inverse property implicitlyLoadedBefore?

@codecov
Copy link

codecov bot commented May 29, 2020

Codecov Report

Merging #3606 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3606      +/-   ##
==========================================
+ Coverage   96.36%   96.39%   +0.02%     
==========================================
  Files         181      182       +1     
  Lines        6168     6210      +42     
  Branches     1814     1820       +6     
==========================================
+ Hits         5944     5986      +42     
  Misses        111      111              
  Partials      113      113              
Impacted Files Coverage Δ
src/Bundle.ts 100.00% <100.00%> (ø)
src/Chunk.ts 99.79% <100.00%> (+<0.01%) ⬆️
src/Graph.ts 100.00% <100.00%> (ø)
src/Module.ts 98.90% <100.00%> (+<0.01%) ⬆️
src/ModuleLoader.ts 100.00% <100.00%> (ø)
src/utils/FileEmitter.ts 100.00% <100.00%> (ø)
src/utils/chunkAssignment.ts 100.00% <100.00%> (ø)
src/utils/error.ts 100.00% <100.00%> (ø)
src/utils/executionOrder.ts 100.00% <100.00%> (ø)
src/utils/getId.ts 100.00% <100.00%> (ø)
... and 2 more

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 5bcb3d5...977b543. Read the comment docs.

@lukastaegert
Copy link
Member Author

Another point I keep stumbling about is how to handle implicit dependant modules that are not part of the graph. We already considered the case where the id does not exist, but another case is where the module is empty or otherwise has no relevant side-effects. In that case, it is not assigned to any chunk and it is unclear which chunk should be loaded before.

One way to handle this would be to throw an error here as well because by the implicit dependencies, there would be no way to reach that chunk.

Alternatively, we could ignore missing implicit dependant modules and if there are none in the end, we promote the chunk to a regular entry point? Any thoughts @LarsDenBakker?

Note that this should not be a problem if a chunks solely depends on entry modules because for those, a chunk is always created, but I need to check.

Another point that is still missing is that our chunks do not yet respect entry signatures.

@lukastaegert
Copy link
Member Author

lukastaegert commented May 31, 2020

How do we handle the case where one of the implicitlyLoadedAfterOneOf modules is not part of the output graph

After some consideration I think I want to throw errors for now. Replacing an error with a non-error logic can always be done in a non-breaking change, the alternative is more difficult.

@lukastaegert lukastaegert force-pushed the gh-3574-implicit-dependencies branch from ae0b74c to 6ca9118 Compare May 31, 2020 19:46
@LarsDenBakker
Copy link
Contributor

Thanks a lot for this! Sorry I haven't had any time to test this yet, will try it out soon.

@lukastaegert lukastaegert force-pushed the gh-3574-implicit-dependencies branch from 8f70e98 to d32a4a1 Compare June 2, 2020 19:29
@lukastaegert
Copy link
Member Author

This is now fully working, all that is missing is some documentation

@lukastaegert lukastaegert force-pushed the gh-3574-implicit-dependencies branch from e847763 to 771a51e Compare June 3, 2020 04:35
@lukastaegert lukastaegert force-pushed the gh-3574-implicit-dependencies branch from 771a51e to 9f21174 Compare June 3, 2020 04:41
@LarsDenBakker
Copy link
Contributor

Tried it out, it works exactly the way I was hoping for 👍

@lukastaegert lukastaegert marked this pull request as ready for review June 3, 2020 05:29
@lukastaegert
Copy link
Member Author

Cool! I also added some documentation now, would be nice if someone could read it and see if it is sufficiently clear. Suggestions of course welcome!

@lukastaegert lukastaegert merged commit c2bd312 into master Jun 3, 2020
@lukastaegert lukastaegert deleted the gh-3574-implicit-dependencies branch June 3, 2020 19:33
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.

Allow specifying implicit dependencies when emitting chunks for better chunking
3 participants