Plugin system refactoring by guybedford · Pull Request #2382 · rollup/rollup

guybedford · 2018-08-06T16:59:55Z

This should be an almost-backwards-compatible refactoring, with some minor error differences that will be fine to release under a new 0.x version here.

Changes:

We create a single plugin driver interface that is used to call the plugin hooks.
In the process we also allow plugins to now have their own individual contexts (opening up the door for each plugin having a unique .cache
Some error handling has been added for plugin name collisions
One big benefit is that now all plugin hook errors get "hook" and "plugin" added to them so that we can always track which plugin caused the original error. This is the user-facing breaking change, and will likely not even be noticed at all.

We can certainly work on improving the output further for coming majors, and hopefully by unifying the code paths this will now be easier.

Closes #2250.

@shellscape should recognise the base plugin approach from Webpack well :)

guybedford · 2018-08-06T17:00:43Z

Note this is based to the asset-refactoring PR branch because that had some needed refactorings for the plugin context interface that were used here.

guybedford · 2018-08-06T17:05:45Z

This also happens to include #2375.

TrySound · 2018-08-06T17:55:16Z

Cool! However it's hard to read with big tabs. Could you also ignore \0 paths or do you consider using driver to handle these cases?

Andarist · 2018-08-06T21:15:19Z

+			this.isExternal = options.external;
+		} else {
+			const ids = ensureArray(options.external);
+			this.isExternal = id => ids.indexOf(id) !== -1;


i realize that this is out of scope of this PR, but maybe this function could be memoized? could get some minor perf boost for bigger projects with a lot of externals

The indexOf operation shouldn't be costly here at all, so the main optimization case would be user-provided isExternal functions. In those cases I think custom memoization might be best in the hook itself?

An alternative would be to change this to ensureSet and call ids.has(id) - if we're concerned with speed.

Andarist · 2018-08-06T21:18:49Z

-
-	const err = new Error(props.message);
+export default function error(base: Error | RollupError, props?: RollupError) {
+	if (base instanceof Error === false) base = Object.assign(new Error(base.message), base);


personally I think if (!(base instanceof Error)) { is less confusing, but ofc its just a personal taste of mine

Andarist · 2018-08-06T21:22:03Z

+			if (existingPluginNames.indexOf(plugin.name) !== -1)
+				error({
+					code: 'DUPLICATE_PLUGIN_NAME',
+					message: `The plugin name ${


while this is true - plugins should have unique names, I'm not sure if you should fail the build in this case - this might cause trouble for rollup users because of a rollup plugin author's mistake

also - it seems reasonable to i.e. use rollup-plugin-babel multiple times (with different babel configs on different paths - using exclude/include options)

Andarist · 2018-08-06T21:26:29Z

Could u also address not always reporting error.stack to the CLI?

Love errors getting more contextual info ❤️

eight04 · 2018-08-06T23:24:59Z

Does it fix #2308?

keithamus · 2018-08-07T12:41:56Z

👍 from me! Looks like this addresses most of #2308 as well, as errors/warnings will get consistent code and plugin values.

shellscape

While I'm not a fan of the patterns that webpack chose in declaring hooks, I think the differences here work well. I'm also a huge fan of this being backwards compatible - not having to programmatically declare which hooks you want to use is a massive plus and good thing™️ for the developer experience.

This may be slight bikeshedding, but food for thought. The use of "hook" to define these is technically accurate by definition for most of the plugin hooks. However, the documentation up to this point has referred to them only as "properties". To provide distinction between Rollup and Webpack, and for clarity about what is technically a hook and what is not technically a hook (hooks seem to be by definition a function that can modify up/downstream), perhaps a different term could be considered. Nothing close to a serious concern, but wanted to toss that bit in there for posterity.

guybedford · 2018-08-07T14:34:51Z

Yes this does fix #2308 as well I believe.

guybedford · 2018-08-07T14:35:30Z

@Andarist help with pinpointing the exact issue you're referring to there would be appreciated.

Andarist · 2018-08-07T15:54:26Z

The issue was that CLI swallows error.stack if error.frame is present. I believe while most users dont care about the stack its very useful for fixing problems upstream and should always be reported. As it being less useful for majority of users im OK with reporting it in grey color or smth (so it doesnt catch ur eye as easily) and keep red for message and frame

guybedford · 2018-08-08T09:43:18Z

@Andarist is that exactly what #2307 is tracking? It should be possible to add the change to the CLI logging here. Would you be interested in making a PR?

…ations

guybedford · 2018-08-08T10:14:53Z

I've rebased this to master, and included a Set fix for ensureArray.

Andarist · 2018-08-08T10:18:53Z

@Andarist is that exactly what #2307 is tracking?

Among other things (some of them are resolved by this PR) - yes! Once this gets published I'm going to check the CLI error outputs, close my issue & open more focused ones (if needed) for stuff that didnt get addressed

It should be possible to add the change to the CLI logging here. Would you be interested in making a PR?

Should I create a PR to this PR? 😄

guybedford · 2018-08-08T10:22:06Z

@Andarist that would be a great help.

Andarist · 2018-08-08T10:36:50Z

I'll probably do it (wanted to do it anyway), not sure if I can manage to do it before this PR gets merged in though. Anyway - it's not blocking anything here, so don't want for me and I'll just do my thing when I get time to do it.

lukastaegert

Now this is a PR that I just love! Especially the abstractions to how to run hooks in a unified way are just awesome!

Just some minor comments here and there.

lukastaegert · 2018-08-11T05:34:40Z

+			let writePromise: Promise<any> = Promise.resolve();
+			for (const output of outputOptions)
+				writePromise = writePromise.then(() => <Promise<any>>bundle.write(output));
+			return writePromise.then(() => bundle);


I like the simplification but I wonder if it is possible to run the writes in parallel via Promise.all or if this may break things. Changing the code did not create any red tests for me but if these are potential ordering issues for hooks, it may be hard to reproduce them in tests.
If there are, we should at least add a comment why this cannot be changed, otherwise I would change to a Promise.all for the performance benefit.

That would be a perf benefit with no observable difference, well spotted, will add.

lukastaegert · 2018-08-11T05:49:23Z

-	parentId: string,
-	isResolved: boolean
-) => Promise<boolean | void> | boolean | void;
+export type IsExternal = (id: string, parentId: string, isResolved: boolean) => boolean | void;


This is potentially breaking in that previously, it seems it was possible that users returned a Promise for the external option which would have propagated here while this is now prevented. However since I could find no documentation for this, I prefer this simplification.

Previously we said in the type that it would permit a Promise, but the implementation didn't actually handle the promise. So this actually corrects the type to match what happens.

lukastaegert · 2018-08-11T05:53:35Z

-
-		return { intro, outro, banner, footer, hash };
-	});
+function strOrFn(strOrFn: string | (() => string | Promise<string>)) {


Maybe normalizeStrOfFn or evaluateIfFn or similar as otherwise, it only describes the argument but not what we do with it. We could also explicitly add the return type to the signature so that it is immediately visible this generates either strings or Promises of string.

Good suggestion.

lukastaegert · 2018-08-11T05:56:11Z

-		].map(({ pluginName, source }, idx) => {
-			if (!source) return;
+const reduceSep = (out: string, next: string) => (next ? `${out}\n${next}` : out);
+const reduceDblSep = (out: string, next: string) => (next ? `${out}\n\n${next}` : out);


I know the name implies it is meant for a reduce but maybe concatSep and concatDblSep would work as well but tell us at a glance what the result is?

lukastaegert · 2018-08-11T05:58:51Z

+			if (typeof specifier === 'string')
+				return <Promise<string>>this.resolveId(specifier, parentId);
+		}
+	};


Really nice idea handling it like this 👍

lukastaegert · 2018-08-11T06:00:39Z

+export function getRollupDefaultPlugin(options: InputOptions): Plugin {
+	return {
+		name: 'Rollup Core',
+		resolveId: resolveId(options),


I wonder if we should rename the rhs function to getResolveId or createResolveId to distinguish it from the actual hook which is the return value of this function?

Would probably trigger some renames in Graph as well, though.

lukastaegert · 2018-08-11T06:13:08Z

+				if (err.code) err.pluginCode = err.code;
+				err.code = 'PLUGIN_ERROR';
+				err.plugin = plugin.name || '(anonymous plugin)';
+				error(err);


Maybe the code to normalize warnings and errors should actually be extracted into a shared function so that this can become something like
error(normalizePluginError('PLUGIN_ERROR')) and `graph.warn(normalizePluginError('PLUGIN_WARNING'))?

Agreed this should be a next step handling error centralization. Would prefer to do that in successive PR work.

lukastaegert · 2018-08-11T06:21:23Z

+						warning.plugin = plugin.name || '(anonymous plugin)';
+						warning.hook = 'transform';
+						graph.warn(warning);
+					},


Again, the code for normalizing warnings and errors looks nearly identical. Maybe it could even be a central utility function to normalize warnings and errors from plugins with parameters to customize it for the respective hook and if it is a warning or error?
That way we ensure the warning/error handling is always consistent across hooks and have a single place to do changes?

Again, I didn't want to get tooo deep into the errors right now. There's a lot that can be done here.

lukastaegert · 2018-08-11T06:27:40Z

+		} else {
+			const external = options.external;
+			const ids = new Set(Array.isArray(external) ? external : external ? [external] : []);
+			this.isExternal = id => ids.has(id);


Definitely an improvement! As all the ids are strings, we could also put them as keys into an object {[id: string]: true} for another very slight performance boost? Might be a micro-optimization, though.

JS sets should be equal or better in performance according to theory.

Definitely looks a little cleaner so let's follow the theory. Googling shows that results are definitely inconclusive which is not necessarily what I would have expected, so the cleaner solution should win here 👍

guybedford · 2018-08-11T09:50:32Z

Thanks for the review! Yes this has been on the list for a while, was glad to have a reason to get to it.

lukastaegert · 2018-08-12T08:05:02Z

 }

-export function createResolveId(options: InputOptions) {
+function createResolveId(options: InputOptions) {


I removed the unnecessary export here which was probably an artefact.

guybedford mentioned this pull request Aug 6, 2018

[question] plugin error reporting #2307

Closed

guybedford mentioned this pull request Aug 6, 2018

Don't test entry points for external #2375

Closed

guybedford requested a review from lukastaegert August 6, 2018 17:10

Andarist reviewed Aug 6, 2018

View reviewed changes

shellscape approved these changes Aug 7, 2018

View reviewed changes

This was referenced Aug 7, 2018

perf(load): memoize default load hook #2386

Closed

perf: use new PluginDriver cache rollup/rollup-plugin-node-resolve#175

Closed

perf: use new PluginDriver cache rollup/rollup-plugin-babel#235

Closed

guybedford added 4 commits August 8, 2018 11:54

asset fixes and refactoring for caching

2f997b8

revert cacheType

5ba6c09

refactor plugin hooks behind a single interface, plugin error augment…

91d7dbf

…ations

ensureArray -> set checking external

82b3213

guybedford force-pushed the plugin-driver branch from 6fb973d to 82b3213 Compare August 8, 2018 10:13

guybedford changed the base branch from asset-refactoring to master August 8, 2018 10:14

guybedford added 2 commits August 8, 2018 13:57

remove name handling and cache code

731475a

Merge branch 'master' into plugin-driver

24bd080

lukastaegert approved these changes Aug 11, 2018

View reviewed changes

guybedford added 2 commits August 11, 2018 11:50

pr feedback

4c0ed1c

Merge branch 'master' into plugin-driver

c19485d

lukastaegert approved these changes Aug 12, 2018

View reviewed changes

lukastaegert and others added 2 commits August 12, 2018 10:03

Remove unnecessary export

492505e

Merge branch 'master' into plugin-driver

ce021e2

lukastaegert reviewed Aug 12, 2018

View reviewed changes

lukastaegert merged commit 2d17db9 into master Aug 12, 2018

lukastaegert deleted the plugin-driver branch August 12, 2018 08:53

sebinsua mentioned this pull request Sep 28, 2018

Regression when resolving the module ids of externalised dynamic imports of NPM packages in rollup@0.66.2 #2481

Closed

Uh oh!

Uh oh!

Conversation

guybedford commented Aug 6, 2018

Uh oh!

guybedford commented Aug 6, 2018

Uh oh!

guybedford commented Aug 6, 2018

Uh oh!

TrySound commented Aug 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andarist commented Aug 6, 2018

Uh oh!

eight04 commented Aug 6, 2018

Uh oh!

keithamus commented Aug 7, 2018

Uh oh!

shellscape left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guybedford commented Aug 7, 2018

Uh oh!

guybedford commented Aug 7, 2018

Uh oh!

Andarist commented Aug 7, 2018

Uh oh!

guybedford commented Aug 8, 2018

Uh oh!

guybedford commented Aug 8, 2018

Uh oh!

Andarist commented Aug 8, 2018

Uh oh!

guybedford commented Aug 8, 2018

Uh oh!

Andarist commented Aug 8, 2018

Uh oh!

lukastaegert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shellscape left a comment •

edited

Loading