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

feat: support async plugins #4671

Merged
merged 12 commits into from Oct 15, 2022
Merged

feat: support async plugins #4671

merged 12 commits into from Oct 15, 2022

Conversation

sxzz
Copy link
Contributor

@sxzz sxzz commented Oct 13, 2022

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:

Description

Add async plugin support

Do not merge this PR before upstream PR #4668 gets merged

@sxzz sxzz changed the title fix: nested plugin in options stage feat: support async plugins Oct 13, 2022
@codecov
Copy link

codecov bot commented Oct 13, 2022

Codecov Report

Merging #4671 (304f750) into master (53de5b0) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4671      +/-   ##
==========================================
- Coverage   99.05%   99.02%   -0.04%     
==========================================
  Files         214      215       +1     
  Lines        7548     7552       +4     
  Branches     2092     2092              
==========================================
+ Hits         7477     7478       +1     
- Misses         23       24       +1     
- Partials       48       50       +2     
Impacted Files Coverage Δ
src/utils/options/normalizeInputOptions.ts 100.00% <ø> (ø)
src/utils/options/normalizeOutputOptions.ts 100.00% <ø> (ø)
cli/run/commandPlugins.ts 93.18% <100.00%> (ø)
cli/run/loadConfigFile.ts 96.61% <100.00%> (ø)
cli/run/loadConfigFromCommand.ts 100.00% <100.00%> (ø)
cli/run/watch-cli.ts 86.11% <100.00%> (-4.43%) ⬇️
src/rollup/rollup.ts 100.00% <100.00%> (ø)
src/utils/asyncFlatten.ts 100.00% <100.00%> (ø)
src/utils/options/mergeOptions.ts 100.00% <100.00%> (ø)
src/utils/options/options.ts 100.00% <100.00%> (ø)
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sxzz sxzz marked this pull request as ready for review October 13, 2022 20:38
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.

Looks great already. Some minor points I found and of course we again need to document it a little.
I keep thinking about the coverage change in watch-cli. I do not think it is something to worry about in your ticket. In the end, we should probably send an exit code 1 to close in the uncaughtException handler.

src/rollup/rollup.ts Outdated Show resolved Hide resolved
| false
| undefined
| InputPluginOption[]
| Promise<Plugin | false | null | undefined | InputPluginOption[]>;
Copy link
Member

Choose a reason for hiding this comment

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

As a mini optimization so we do not need to repeat everything, maybe use either an intermediate type so that

type InputPluginOption = InputPluginOptionValue | Promise<InputOptionValue>

or introduce a utility type like

type MaybePromise<T> = T | Promise<T>;

that could also be reused in other places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I'll make more PR to improve and simplify types.

src/rollup/types.d.ts Outdated Show resolved Hide resolved
array = (await Promise.all(array)).flat(Infinity) as any;
} while (array.some((v: any) => v?.then));
return array;
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder it would not make sense to integrate the filter(Boolean) here as well so you do not need the optional chaining in v?.then? But then maybe this should just become normalizePluginOption?

Copy link
Contributor Author

@sxzz sxzz Oct 14, 2022

Choose a reason for hiding this comment

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

This function is copied from Vite.

I think we just keep the function simple and keep the falsy value (the fn is not referenced by now, but in the future, it could be used in other places). filter(Boolean) is done in normalizePluginOption.

src/watch/watch-proxy.ts Outdated Show resolved Hide resolved
@sxzz
Copy link
Contributor Author

sxzz commented Oct 14, 2022

I didn't get your point of we should probably send an exit code

Are there any chances to emit uncaughtException by Rollup? As we can see, it will catched by handleError and process.on

watchInternal(configs, emitter).catch(error => {
handleError(error);
});

process.on('uncaughtException', close);

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.

Looks good to be merged, thanks a lot!

@lukastaegert lukastaegert merged commit 585de6d into rollup:master Oct 15, 2022
@rollup-bot
Copy link
Collaborator

This PR has been released as part of rollup@3.2.0. You can test it via npm install rollup.

@lukastaegert
Copy link
Member

I didn't get your point of we should probably send an exit code

Are there any chances to emit uncaughtException by Rollup? As we can see, it will catched by handleError and process.on

Sorry, missed that last question. Yes, it is handled, but it does

process.on('uncaughtException', close); 

async function close(code: number | null): Promise<void> {
		process.removeListener('uncaughtException', close);
		// removing a non-existent listener is a no-op
		process.stdin.removeListener('end', close);

		if (watcher) await watcher.close();
		if (configWatcher) configWatcher.close();

		if (code) {
			// eslint-disable-next-line unicorn/no-process-exit
			process.exit(code);
		}
	}

which means no explicit exit code is passed to close, instead it passes the event as code. That means process.exit is triggered with a non-number code, which is interpreted as process.exit(0). But maybe this is to be resolved another time.

@sxzz sxzz deleted the feat/async-plugins branch October 15, 2022 06:34
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

3 participants