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

[rush] Basic support of --watch for BulkScriptAction (build, rebuild, etc.) #2458

Merged
merged 19 commits into from
Feb 11, 2021

Conversation

dmichon-msft
Copy link
Contributor

@dmichon-msft dmichon-msft commented Jan 29, 2021

Summary

Globally adds --watch support to BulkScriptAction, thereby including rush build, rush rebuild, and any user-defined commands that may do so more efficiently (for example someone may define rush build:incremental).

This allows any repository that defines npm run build to get the full benefit of automatic rebuild in response to source code changes, without regard to what tool is used for the individual project's build command.

Works best if the npm run build (or alternative command) script is optimized for efficient incremental rebuild with disk caching. Consider TypeScript 4, Webpack 5, etc.

Fixes #1122
Addresses "Feature 1" from #1202.

Details

Handling of --watch

Support for --watch is implemented in BulkScriptAction by evaluating the command line parameters to determine the selected projects, then invoking either a single execution or watch mode.
Watch mode is implemented as a basic loop:

  1. Wait for one or more projects to have different inputs than the last recorded state, then record the state (as determined by PackageChangeAnalyzer). Initially there is no recorded state, so this returns immediately.
  2. Run the underlying command, filtered to the changed projects and their dependents, filtered to the initial selection from the enclosing command
  3. Goto (1)

Change detection is implemented as follows:

  1. Instantiate a new PackageChangeAnalyzer and compare the hashes for the selected projects with the previous (in-memory) execution state. If any projects contain changed hashes, skip directly to (4).
  2. Instantiate a file system watcher using chokidar for the root folders of all selected projects. Any changes trigger a debounced check identical to (1), until such time as one or more projects report changes.
  3. Shut down the file system watcher to save resources and avoid noise during build
  4. Return the set of changed projects and the new state (PackageChangeAnalyzer) to hand to TaskSelector

Backwards compatibility concerns:

  • If, for some reason, developers were trying to forward --watch through a BulkScriptAction to underlying npm scripts, that will no longer work.

Currently includes the changes from #2422, will be rebased if that goes in. The Selection manipulation and refactoring of TaskSelector were relevant to implementation.

Alternatives considered:

  • Communicate with individual package builder processes in watch mode. This makes demands on how developers implement npm run build, and may produce faster results in certain optimal environments, but limits the general utility of the feature.
  • Use a separate action altogether, e.g. rush build:watch. This makes parameter forwarding a bit awkward and would require either architectural changes in command-line.json for invoking an npm command with a different name than the rush command or for developers to supply an additional npm command definition to build. Not infeasible, but is a separate issue and alters the ergonomics of the command line. The current implementation conforms to the contract that rush build --foo --bar has the same behavior as rush build --foo --bar --watch with respect to building each project.

Future improvements:

  • Canceling extant build queue and immediately beginning a new build upon subsequent file changes to minimize total end to end time. This involves passing a cancellation token of some sort to TaskRunner, which currently has no precedence in the rushstack repo. Also requires the file system watcher to continue running during the build process, though this has not been shown to cause any particular problems.
  • Phased and/or sharded builds are entirely within the domain of the TaskSelector and TaskRunner and should require no additional work with this implementation

How it was tested

Invoked rush build --to @microsoft/rush-lib --watch against rushstack repo and observed rebuild of the changed project and downstream projects when changes were saved to disk.
Performing further file system changes during execution results in execution occurring immediately after the previous build pass completes.

Edit: Now that #2422 is in, contains only the directly relevant changes.

@octogonz
Copy link
Collaborator

octogonz commented Feb 9, 2021

1. Wait for one or more projects to have different inputs than the last recorded state, then record the state (as determined by `PackageChangeAnalyzer`). Initially there is no recorded state, so this returns immediately.

@dmichon-msft As I recall PackageChangeAnalyzer has trouble with these cases:

  • A file that is tracked by Git, but which has been deleted locally (and not committed yet)
  • A newly created file that has not been added to Git yet

Might be useful to test these edge cases, and if they don't work, we can document that.

@octogonz
Copy link
Collaborator

I updated the CLI docs, and fixed some wording in the change log that was referring to internal implementation details. If you're cool with my changes, let's merge this. 👍

Thanks for implementing this -- I can't wait to use it!

@octogonz octogonz changed the title [rush-lib] Basic support of --watch for BulkScriptAction (build, rebuild, etc.) [rush] Basic support of --watch for BulkScriptAction (build, rebuild, etc.) Feb 10, 2021
apps/rush-lib/package.json Show resolved Hide resolved
apps/rush-lib/src/cli/scriptActions/BulkScriptAction.ts Outdated Show resolved Hide resolved
apps/rush-lib/src/logic/ProjectWatcher.ts Outdated Show resolved Hide resolved
apps/rush-lib/src/logic/ProjectWatcher.ts Outdated Show resolved Hide resolved
apps/rush-lib/src/logic/ProjectWatcher.ts Outdated Show resolved Hide resolved
apps/rush-lib/src/logic/ProjectWatcher.ts Show resolved Hide resolved
common/changes/@microsoft/rush/watch_2021-01-29-00-52.json Outdated Show resolved Hide resolved
@iclanton iclanton merged commit 03eba0f into microsoft:master Feb 11, 2021
@wbern
Copy link
Contributor

wbern commented Feb 11, 2021

Awesome work!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[rush] make incremental build happen on file changes (watch mode)
5 participants