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: CLI event hook flags #4457

Merged
merged 14 commits into from May 5, 2022
Merged

Conversation

Harris-Miller
Copy link
Contributor

@Harris-Miller Harris-Miller commented Apr 5, 2022

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • will soon
  • 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

Overview

When in watch mode, it is not uncommon to want to kick off other tasks at the start or end of a bundle. The built-in CLI already has callbacks set up for the 5 events. This MR sets up config and cli flags to be able to set up custom hooks to run on those events in addition to the current operations.

This MR is incomplete on purpose. Before writing tests and updating documentation, I wanted to see if this feature is something the core team would like. See below for list of TODOs

Motivation

I work I use yalc heavily when developing locally. We use rollup as well. Right now are running a separate rollup -c --watch and chokidar dist -c 'yalc push' to marry incremental builds from rollup and moving the results of that build to other libs via yalc.

The chokidar watch is not necessary though because rollup has event hooks for 'START', 'END', etc. Except the built-in CLI currently only uses those hooks to display build information. It would be great to run yalc push, or any terminal command for that matter, at any of these hook points via the built-in CLI

Current Implementation

Using the systems already in place, I added the following CLI flag:

--watch.onError
--watch.onStart
--watch.onStartBundle
--watch.onEndBundle
--watch.onEnd

Each value is passed to child_process.execSync. To be run. stdio is set to inherit unless --silent is set, then it is ignore. I choose to use execSync, but I'm not opposed to using exec or spawn either, whatever the rollup team would like, just let me know what would be preferred here.

I specifically choose for these to be CLI options only since the intention is to add terminal commands to the built-in CLI. These would never apply to WatchOptions passed in API, eg const watcher = rollup.watch(watchOptions);, as users writing scripts utilizing the API would implement their own watch.on('event') to do what they needed

TODO

As mentioned before, I wanted to gauge if the rollup team would be open to accepting this MR before I completed it

  • Tests
  • Documentation

Final Thoughts

Thank you for the consideration of this MR. IMHO this feature is the main thing missing from the CLI. The level of effort it too to add this feature was minimal, yet it adds a lot of power to users.

@Harris-Miller Harris-Miller marked this pull request as ready for review Apr 5, 2022
Copy link
Member

@lukastaegert lukastaegert left a comment

Actually I think this is an interesting idea with minimal impact. If you add tests and documentation, I would merge this. execSync sounds fine to me as I think this would help the expectation that the command can be executed at exactly the point in time where the event happens, and we also do not expect unrelated tasks to share the event loop.

@codecov
Copy link

@codecov codecov bot commented Apr 7, 2022

Codecov Report

Merging #4457 (56b52c0) into master (f44a3a3) will decrease coverage by 0.03%.
The diff coverage is 85.00%.

@@            Coverage Diff             @@
##           master    #4457      +/-   ##
==========================================
- Coverage   98.75%   98.72%   -0.04%     
==========================================
  Files         205      206       +1     
  Lines        7331     7351      +20     
  Branches     2082     2086       +4     
==========================================
+ Hits         7240     7257      +17     
- Misses         33       34       +1     
- Partials       58       60       +2     
Impacted Files Coverage Δ
cli/run/watchHooks.ts 78.57% <78.57%> (ø)
cli/run/watch-cli.ts 90.54% <100.00%> (+0.83%) ⬆️

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 f44a3a3...56b52c0. Read the comment docs.

@Harris-Miller
Copy link
Contributor Author

@Harris-Miller Harris-Miller commented Apr 13, 2022

lukastaegert Cool cool. I'm going to try and carve out some time next weekend or the weekend after to follow up on this MR and add tests and documentation. I'll ping you when that work is done for you to re-review. Thank you!

bundles main.js → _actual...
watch.onBundleStart $ echo bundleStart
bundleStart
created _actual in 16ms
Copy link
Member

@lukastaegert lukastaegert Apr 30, 2022

Choose a reason for hiding this comment

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

A CLI test must not depend on things like runtime. The usual solution is to use assertIncludes for substrings like created _actual etc.

Copy link
Member

@lukastaegert lukastaegert Apr 30, 2022

Choose a reason for hiding this comment

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

Another idea: Split the output by line, filtering empty ones and mapping the remaining ones to their first word. Then you would just need to compare

start
bundles
bundleStart
created
...

module.exports = {
description: 'onStart event hoot shell command executes correctly',
command:
'rollup -cw --watch --watch.onStart "echo start" --watch.onBundleStart "echo bundleStart" --watch.onBundleEnd "echo bundleEnd" --watch.onEnd "echo onEnd" --watch.onError "echo onError"',
Copy link
Member

@lukastaegert lukastaegert Apr 30, 2022

Choose a reason for hiding this comment

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

-cw includes --watch 😉

Copy link
Contributor Author

@Harris-Miller Harris-Miller May 1, 2022

Choose a reason for hiding this comment

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

lol oops.

I could use some guidance on how to do this test exactly. The test/cli/samples/ suite of test all seem to do tests on either console output or bundle file output. the one I threw up there was just a POC to test all of the watch event hooks at once by just looking for the echos being written

I can get that test working or I can take another direction. Would you like me to break up each hook into its own test? should I test that --silent doesn't print to stderr?

Copy link
Member

@lukastaegert lukastaegert May 1, 2022

Choose a reason for hiding this comment

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

No, I think the test is fine in principle, and testing all hooks together seems adequate to me considering me that there is not too much logic involved. You should just make sure to make it a little more robust, e.g. see my other suggestion of preprocessing the output by splitting it into lines, doing some mapping and filtering and possibly doing an deepStrictEqual on the result. Looks to me like "echo" should also work on Windows, which is definitely a bonus.

@Harris-Miller
Copy link
Contributor Author

@Harris-Miller Harris-Miller commented May 1, 2022

I have 2 passing tests (had to test onError separately). They both take a very long time though

✔ watch-event-hooks: event hook shell commands write to stderr (40014ms)

I added in this that I found in a similar test file, though it doesn't help:

abortOnStderr(data) {
	if (data.includes('waiting for changes')) {
		return true;
	}
}

What am I doing wrong?

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented May 1, 2022

Looks like a process is not terminating correctly and needs to be force-quit by the test framework after the 40s timeout

@Harris-Miller
Copy link
Contributor Author

@Harris-Miller Harris-Miller commented May 2, 2022

I figured out the missing piece for the testing not exiting on abortOnStderr. All good now. I think this completes the requirements. Let me know if you need anything else

@Harris-Miller Harris-Miller changed the title CLI event hook flags feat: CLI event hook flags May 2, 2022
Copy link
Member

@lukastaegert lukastaegert left a comment

Looks great! One last thing I wondered about is error handling, but maybe in watch mode to just print errors but not abort the build should be fine.

@lukastaegert lukastaegert merged commit 0c75914 into rollup:master May 5, 2022
10 of 12 checks passed
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