Plugin API adjustments for chunking #2126

guybedford · 2018-04-16T13:01:03Z

So I've basically implemented the transformChunk hook ideas here from #2043, although with the following differences:

transformBundle remains exactly as it is, except taking a new argument chunk.
transformChunk is an exact alias for transformBundle
I've added two new plugin hooks, onbuildstart and onbuildend, which are synchronous. This is important for lifecycle handling in plugins - my particular use case here is being able to create a worker pool in onbuildstart, and clean up the pool in onbuildend, otherwise we have no idea when to do the cleanup work and the process stalls indefinitely as the workers stay open.
I've extended the context of the plugin like the transform plugin to the other plugins. This allows other plugin hooks to access this.error, this.warn, this.parse and this.resolveId. This starts to refactor some of the plugin hooking, but I didn't go too deep into it. Hopefully we can leave the door open to further refactoring that could perhaps be based on a more generic "plugin driver" for generalised error messages / warnings etc instead of having the logic repeated everywhere, but this was a bit too much to attempt here.
I've also refactored some of the repetitions in rollup/index.ts as well here.

This supersedes #2115.

None of these changes are breaking, and then hopefully a further deprecation release can remove transformBundle entirely with transformChunk replacing it (which is really just a name change).

With this extra chunk argument I think we should be set for CSS code splitting techniques in plugins.

guybedford · 2018-04-17T15:11:28Z

I've added two additional plugin hooks here as well - onbuildstart and onbuildend.

The reason I needed these is for worker pooling in plugins, which needs the ability to track when to create the worker processes and when to shut them down again, otherwise the process will hang and not be able to close down.

guybedford · 2018-04-17T15:13:07Z

src/rollup/index.ts

+								timeEnd('GENERATE', 1);
+
+								const output = {
+									name: relative(process.cwd(), resolve(outputOptions.file || inputOptions.input)),


I can't remember if process is ok to use in the browser or not, would be good to check this.

It's not, accessing process throws an error 😉. But then I am not exactly sure what it should return in a browser...

Thanks, I've added a detection here. Yes the name becomes somewhat guesswork on the edge cases...

guybedford · 2018-04-17T22:09:36Z

src/rollup/index.ts

 											chunks.map(chunk => {
 												return chunk.render(outputOptions, addons).then(rendered => {
 													const output = {
+														name: chunk.id,


We could possibly call this filename as well. The idea is that it is possible to determine the output file path from this using path.resolve(outputOptions.dir || process.cwd(), chunk.name) within the transformChunk hook to reliably output eg a CSS file per chunk,

Looking through our options, I guess the best name would actually be file as this is the output option it would correspond to in the single input case. name is already used for something else.

Another reason we might still want a name when not writing is because we do have file names for chunking, but its only the single file build that doesn't have a rule here. So if we want the APIs to be unified at some point, ideally we can always assign a name - so that was the thinking behind using the input fallback.

Will change to file for now and keep it always-populated, but we can reconsider this.

The other thing is name aligns with the naming of chunkNames and entryNames which are actually naming files in the same space... so would be good to try converge on terminology here actually.

lukastaegert · 2018-04-18T05:59:08Z

Looking forward to this one, hope to give it a proper review soon.

lukastaegert

Thanks for the great work, especially the unified plugin context is something we needed for quite some time. Some thoughts:

The hooks that are attached to graph should all be bound to the right context. It does not make sense to have to remember everywhere that they need to be called with .call(graph.pluginContext,...) and this is easily forgotten in the future.
Speaking of breaking functionality in the future, there are no tests for the new plugin context functions as well as the new hooks
Right now, the plugin hooks are a mixture of alllowercasenames and camelCasedNames. I know that ongenerate and onwrite created some precedence here but my suggestion would actually be to go the opposite way as plugin hooks are near impossible to change (though we might have a chance with the 1.0 release if we communicate it well):
- use camel case for the onBuildStart and onBuildEnd hooks
- create aliases onGenerate and onWrite for the existing hook, the same way it is done for the transformBundle hook
- maybe we can deprecate the all lowercase names with the 1.0 (but then we should start warning about their usage today to give people a change to update)

lukastaegert · 2018-04-23T08:18:04Z

src/Module.ts

@@ -634,7 +634,8 @@ export default class Module {
 			const declaration = otherModule.traceExport(importDeclaration.name);

 			if (!declaration) {
-				this.graph.handleMissingExport(
+				this.graph.handleMissingExport.call(
+					this.graph.pluginContext,


It seems to me overly complicated and redundant to always hand the handler on this.graph its own this.graph.pluginContext.

As performance is probably not an issue for this handler, I would suggest to instead let the graph itself take care of using the right context (e.g. via wrapping the handler appropriately) so that modules do not need to care about something that is of no concern to them.

lukastaegert · 2018-04-23T08:22:23Z

src/rollup/index.ts

-	write: (options: OutputOptions) => Promise<{ [chunkName: string]: OutputChunk }>;
-	getTimings?: () => SerializedTimings;
-}
-


lukastaegert · 2018-04-23T08:59:46Z

src/rollup/index.ts

+								};
+
+								return Promise.all(
+									graph.plugins


So with this change, generate will now wait for all ongenerate hooks that return a promise instead of just going on. Probably makes sense (one can just not return a Promise to not stall the generate call) but we should definitely not forget to document this on the plugin wiki!

Yes, until now only onwrite could return a promise.

lukastaegert · 2018-04-23T09:04:31Z

src/rollup/index.ts

+								});
+							}
+							return generate(outputOptions).then(result => {
+								return writeChunk(graph, outputOptions.file, result, outputOptions).then(


Nice extraction!

Looks even nicer here if you shorthand the arrow function 😉

Would be even nicer if we can finally update our source to async/await... getting Rollup itself on autocompile is a near-term goal towards this, but this PR has to land first I guess :)

lukastaegert · 2018-04-23T09:10:31Z

src/rollup/index.ts

+
+	return writeFile(filename, code)
+		.then(() => writeSourceMapPromise)
+		.then(() => {


lukastaegert · 2018-04-23T09:44:58Z

src/rollup/types.d.ts

-	footer?: string;
-	intro?: string;
-	outro?: string;
+	banner?: string | (() => string | Promise<string>);


Good catch!

Just wondering, even though this is not a plugin hook, isn't the signature still AddonHook, i.e. aren't we using the graph context when calling anyway?

lukastaegert · 2018-04-23T09:46:51Z

src/utils/addons.ts

@@ -30,40 +28,36 @@ export function createAddons(graph: Graph, options: OutputOptions): Promise<Addo

 function collectAddon(
 	graph: Graph,
-	initialAddon: string,
+	initialAddon: string | (() => string | Promise<string>),


Again, isn't this just AddonHook as we are definitely calling it using the right context?

Yeah these repeats are all down to the context typing issue - there's no way for a bound function to have the same type as the unbound version without using .call.

lukastaegert · 2018-04-23T09:50:55Z

src/utils/transform.ts

 	RawSourceMap
 } from '../rollup/types';
 import Program from '../ast/nodes/Program';
-import { TransformContext } from '../rollup/types';
+
+function augmentCodeLocation<T extends RollupError | RollupWarning>({


Very nice extractions!

lukastaegert · 2018-04-23T09:57:57Z

src/Graph.ts

@@ -130,7 +157,13 @@ export default class Graph {
 						importedModule: string,
 						importerStart?: number
 					) => {
-						return missingExport(importingModule.id, exportName, importedModule, importerStart);
+						return missingExport.call(


Looking at this, it seems to me there is no reason at all why handleMissingExport is called in Module using the graph context.

The same goes for the other hooks that are attached to graph: resolveDynamicImport, isExternal, ...

lukastaegert · 2018-04-23T10:01:29Z

src/Graph.ts

 				.concat(resolveId(options))
 		);

+		this.pluginContext.resolveId = this.resolveId;
+
 		const loaders = this.plugins.map(plugin => plugin.load).filter(Boolean);


I would prefer adding the context to the loaders here instead of having to remember this everywhere where this.load is called, or graph.load for that matter.

With regards to this feedback on having the context bound - the issue here is that we then have to duplicate the types of everything as one type will have the 'this' binding and one type without. Personally I think the type maintenance is more work than just applying the right call, and TypeScript will error if the right call context isn't provided as well.

guybedford · 2018-04-23T16:38:11Z

Yes I will pad out tests, let's first keep the review moving on the functionality if you don't mind? This PR is kind of tracking what I find I need to get a chunked CSS workflow going... I might try and sneak in a super simple asset emission API as well in the next couple of days (I wasn't going to do this, but it is useful for in-memory builds for eg SSR use cases). Then will lock down tests when we're agreed on functionality.

Better than running the context setup through 'bind' I think it might even be easier to simply use a plugin driver instead of calling hooks directly. Would also help avoid duplicate typing issues. Let me see if I can make some progress on this.

If we're doing renaming of onWrite and onGenerate, perhaps we can drop the "on" and just make the plugin hooks write, generate, buildStart and buildEnd? Or if we want to be more descriptive, perhaps writeChunk and generateChunk?

guybedford · 2018-04-23T17:04:16Z

If we're going with file on chunks, I'd be tempted to rewrite entryNames and chunkNames as entryFileNames and chunkFileNames to be clearer here. Let me know your thoughts though.

lukastaegert · 2018-04-28T21:51:59Z

if we're doing renaming of onWrite and onGenerate, perhaps we can drop the "on" and just make the plugin hooks write, generate, buildStart and buildEnd? Or if we want to be more descriptive, perhaps writeChunk and generateChunk

Yes, I think the more concise form without "on" looks good for the hooks. writeChunk and generateChunk would be ok with me, leaves room for more write* and generate* hooks in the future.

If we're going with file on chunks, I'd be tempted to rewrite entryNames and chunkNames as entryFileNames and chunkFileNames to be clearer here.

Sounds good to me!

guybedford · 2018-05-01T19:38:55Z

@lukastaegert just a quick update here - I'm getting pulled back into some other work for a bit, so I'm having to put this plugin and code splitting unification work on hold for at most a month. Will keep you posted when I'm back on this. The other PRs should all be good to land for now though.

…cImports option for single-file

guybedford · 2018-05-18T15:45:44Z

I've finally got back to updating this PR. It also includes the code splitting refactoring and unification path now as well as well as a much simpler asset API than originally planned.

To summarize (repeating from above as well):

transformBundle has been renamed to transformChunk taking one extra argument - the chunk itself
I've added buildStart and buildEnd hooks, that apply for the rollup.rollup lifecycle, regardless of generate or write calls.
entryNames and chunkNames are now entryFileNames and chunkFileNames.
A new assetFileNames option is added
Code splitting is refactored into a single path based on a inlineDynamicImports input option.

`generateBundle` hook

I've created a new plugin hook - generateBundle that gets the object map of output file names to chunks. This hook is allowed to modify this object, including defining additional files that will be written by write or returned by generate. Assets are then entries that are either source strings or instances of Buffer.

The signature of this hook is generateBundle (outputOptions, outputBundle, getAssetFileName(name: string, source: string) => string, isWrite: boolean).

The getAssetFileName is mostly just a suggested convention for asset emission and allows avoiding a custom emission API to instead just allow users to define their own assets.

Basically it is intended to be used like:

generateBundle (outputOptions, outputBundle, getAssetFileName) {
  const assetSource = 'custom source';
  const assetFileName = getAssetFileName('custom.ext', assetSource);
  outputBundle[assetFileName] = assetSource;
}

so it is super rudimentary and manual in that it is just trying to encourage a pattern rather than take over - the user still populates the asset themselves. The asset names are deduped as well so if there are conflicts those are automatically handled with indexing.

The asset name pattern can then be set with outputOptions.assetFileNames, where the default is assets/[name]-[hash][ext]. Of course plugin authors can bypass this all entirely if they want too and just write their own files.

Code Splitting non-experimental path

This is obviously all still under discussion, but the suggestion here for supporting code splitting in non-experimental is then to:

Support multiple inputs by default
Have the bundle and generate Rollup methods return a map of files instead of a single { code, map } object.
inlineDynamicImports automatically assumed true for a single input string, false otherwise

We could even consider a codeSplitting: true / false option where multiple inputs can support non-code-splitting.

Deprecation paths (possible 1.0)

I'd like to encourage some deprecation paths here as well, and again this is all open to discussion:

Remove transformBundle for transformChunk.
Deprecate onwrite and ongenerate for the single generateBundle which can handle all these cases, as it's last argument is an isWrite boolean.

@lukastaegert please review again when you have time it would be nice to finally move this one forward again. Also glad to discuss here or in a separate issue the details of the approaches, upgrade paths and deprecations, there's a lot to bring together here.

guybedford · 2018-05-18T16:33:40Z

Unfortunately I messed up the core use case here entirely of being able to substitute asset URLs... will revise next week - ignore for now.

guybedford · 2018-05-21T15:09:34Z

Closing to reopen with the new approach.

guybedford requested a review from lukastaegert April 16, 2018 13:06

guybedford mentioned this pull request Apr 16, 2018

Pass codeSplitting generated filenames to onwrite hooks #2115

Closed

guybedford commented Apr 17, 2018

View reviewed changes

guybedford mentioned this pull request Apr 17, 2018

How to configure rollup watcher handlers? #2117

Closed

guybedford force-pushed the plugin-chunking branch 2 times, most recently from f4c887d to aa328c5 Compare April 17, 2018 22:07

guybedford commented Apr 17, 2018

View reviewed changes

guybedford force-pushed the plugin-chunking branch from a78856a to 1978df8 Compare April 18, 2018 13:54

guybedford mentioned this pull request Apr 22, 2018

unify MISSING_EXPORT calls #2147

Closed

lukastaegert requested changes Apr 23, 2018

View reviewed changes

guybedford mentioned this pull request Apr 27, 2018

Show errors for every build in multiconfig #2162

Closed

eight04 mentioned this pull request May 2, 2018

Switch to pluginContext.resolveId eight04/rollup-plugin-cjs-es#1

Closed

guybedford added 12 commits May 18, 2018 13:49

plugin api ajustments

95db778

unified plugin interface, pluginContext

5e61a2e

typing fixes

50a78b0

add onbuildstart and onbuildend hooks

dcf466f

sync-only onbuildstart, onbuildend

f885c53

onbuilderror hook

fd6a828

update onbuildend hook to take error argument

9de90a5

update transformChunk return type

52524f0

ensure name on OutputChunk and remove repeated type declarations

d1e0e0a

transformChunk argument

b3590ca

handle process detection, remove unused types

91c5dc4

update transformChunk return value types

5c0d6ca

guybedford added 5 commits May 18, 2018 13:50

feedback wip

8f47cd3

asset emission API, unify code-splitting, single-file codepath

99a080c

complete asset emission API

73480d7

experimentalCodeSplitting to always return files object, inlineDynami…

158d881

…cImports option for single-file

generate assets first to enable inlining

e955e09

guybedford mentioned this pull request May 18, 2018

[Roadmap] The future of experimentalCodeSplitting and experimentalDynamicImports #2092

Closed

16 tasks

api refactoring, very simple asset handling

1de6b19

guybedford force-pushed the plugin-chunking branch from e33c418 to 1de6b19 Compare May 18, 2018 15:30

guybedford mentioned this pull request May 18, 2018

Asset Emission API #2163

Closed

guybedford closed this May 21, 2018

guybedford mentioned this pull request May 22, 2018

Build and plugin hooks refactoring for code splitting and asset emission workflows #2208

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin API adjustments for chunking #2126

Plugin API adjustments for chunking #2126

guybedford commented Apr 16, 2018 •

edited

guybedford commented Apr 17, 2018

guybedford Apr 17, 2018

lukastaegert Apr 18, 2018

guybedford Apr 18, 2018

guybedford Apr 17, 2018 •

edited

lukastaegert Apr 23, 2018

guybedford Apr 23, 2018

guybedford Apr 23, 2018

lukastaegert commented Apr 18, 2018

lukastaegert left a comment

lukastaegert Apr 23, 2018

lukastaegert Apr 23, 2018

lukastaegert Apr 23, 2018

guybedford Apr 23, 2018

lukastaegert Apr 23, 2018

guybedford Apr 23, 2018

lukastaegert Apr 23, 2018

lukastaegert Apr 23, 2018

lukastaegert Apr 23, 2018

guybedford Apr 23, 2018

lukastaegert Apr 23, 2018

lukastaegert Apr 23, 2018

lukastaegert Apr 23, 2018

lukastaegert Apr 23, 2018

guybedford May 21, 2018

guybedford commented Apr 23, 2018

guybedford commented Apr 23, 2018

lukastaegert commented Apr 28, 2018

guybedford commented May 1, 2018

guybedford commented May 18, 2018

guybedford commented May 18, 2018

guybedford commented May 21, 2018

Plugin API adjustments for chunking #2126

Plugin API adjustments for chunking #2126

Conversation

guybedford commented Apr 16, 2018 • edited

guybedford commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford Apr 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukastaegert commented Apr 18, 2018

lukastaegert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Apr 23, 2018

guybedford commented Apr 23, 2018

lukastaegert commented Apr 28, 2018

guybedford commented May 1, 2018

guybedford commented May 18, 2018

generateBundle hook

Code Splitting non-experimental path

Deprecation paths (possible 1.0)

guybedford commented May 18, 2018

guybedford commented May 21, 2018

guybedford commented Apr 16, 2018 •

edited

guybedford Apr 17, 2018 •

edited

`generateBundle` hook