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] Manage multiple autoinstallers more easily #3789

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

elliot-nelson
Copy link
Collaborator

@elliot-nelson elliot-nelson commented Nov 28, 2022

Summary

A small pile of quality-of-life improvements for dealing with autoinstallers:

  • rush update-autoinstaller --name folder1 --name folder2
  • rush update-autoinstaller --all
  • rush update --autoinstallers
  • rush install --autoinstallers
  • All autoinstaller human-readable output now refers to lockfiles instead of shrinkwrap files.
  • Other minor output improvements and bug fixes.

Fixes #3743.

Details

Updating multiple autoinstallers

Following in the vein of rush add, you can now provide the --name parameter multiple times when updating an autoinstaller. Useful for specifically targeting 2 or more autoinstaller folders.

The output of the update-autoinstaller command has been updated to provide a summary of the autoinstallers that were updated and what new lockfiles should be committed to source control.

rush update-autoinstaller --name rush-plugins --name rush-prettier

Updating all autoinstallers

When your monorepo reaches 4-5 autoinstallers, it becomes easier to simply "mass update" them and evaluate the result of the command (ensuring it matches your expectations) rather than typing out specific folder names.

rush update-autoinstaller --all

Convenience method: update + autoinstaller update

After a maintenance update frenzy, with multiple package.json changes spread across project and autoinstaller folders alike, a single command to make sure everything is updated is a nice time saver.

rush update --autoinstallers

# Roughly equivalent to:
rush update && rush update-autoinstaller --all

New functionality: prepare all autoinstallers

A sibling command for rush update --autoinstallers, rush install --autoinstallers is unique in that it provides a feature that doesn't exist in Rush today: a way to prepare all of your autoinstallers for use (without waiting until someone actually calls a related rush global command defined in command-line.json).

This is very useful for situations where processes outside of Rush's control expect autoinstallers to be up-to-date. A great example is the common common/autoinstallers/rush-prettier autoinstaller, which might also be used by VSCode, WebStorm, etc. If you've upgraded prettier or added new prettier plugins to your autoinstaller, rush install --autoinstallers can immediately fix "Missing plugin" errors for your VSCode developers.

How it was tested

  • Tested installs and updates, with and without autoinstallers, in the rushstack repo (and local monorepo)
  • Tested update-autoinstaller with all and with multiple names in local monorepo (and rushstack repo)

Example screenshots

"rush update --autoinstallers":
Screen Shot 2022-11-28 at 5 26 46 PM

@elliot-nelson elliot-nelson changed the title [rush] Autoinstaller "all" functionality [rush] Manage multiple autoinstallers more easily Nov 28, 2022
@isc30
Copy link
Contributor

isc30 commented Nov 29, 2022

Hey, the PR looks good. I would go 1 step further tho, I would:

  • remove update-autoinstaller in favour of plain rush update
  • remove --autoinstallers flag from rush update
  • keep the --autoinstallers flag from rush install

With this, the only options will be:

  • run rush update to update all the changed packages and autoinstallers
  • run rush install to install the dependencies of your packages (and let autoinstallers do a lazy install)
  • run rush install --autoinstallers to install all the dependencies of your packages and autoinstallers (not lazy)

@elliot-nelson
Copy link
Collaborator Author

@isc30 I want to be careful about rush update-autoinstaller because this does have an important use: the way Rush loads Rush plugins is through an autoinstaller, which means you have to have a way to update autoinstallers that is outside the normal bounds of install/update/build (because plugins can actually modify the behavior of those commands). You could end up in a state where a bad autoinstall breaks your ability to fix your autoinstaller.

I also think rush update and rush update-autoinstaller tend to have different audiences... typically only your monorepo maintainers, plus a small set of advanced users, ever touch autoinstallers (usually to set up custom global commands). Whereas application developers in your repo often run rush update, because they are constantly updating the dependencies of their own projects, merging changes from main that have updates and fixing their lockfile, etc. I wouldn't want the application developer workflow to get too muddied up with the maintainer workflow, if I can avoid it.

Hey, the PR looks good. I would go 1 step further tho, I would:

  • remove update-autoinstaller in favour of plain rush update
  • remove --autoinstallers flag from rush update
  • keep the --autoinstallers flag from rush install

With this, the only options will be:

  • run rush update to update all the changed packages and autoinstallers
  • run rush install to install the dependencies of your packages (and let autoinstallers do a lazy install)
  • run rush install --autoinstallers to install all the dependencies of your packages and autoinstallers (not lazy)

@isc30
Copy link
Contributor

isc30 commented Nov 29, 2022

okay then it sounds good, I like having the option to do rush update --autoinstallers as a shortcut for all of them

@elliot-nelson
Copy link
Collaborator Author

@isc30 I was looking at your example rush hook again and it feels like there is something missing there -- you want your repo to install all autoinstallers whenever people run install, and had to write custom hooks to do that.

I think there's two possible approaches to fix that:

(1) We could also add the missing command "install-autoinstaller" (just like update-autoinstaller, but just runs prepare).

If this existed, you'd be able to get rid of your custom javascript script and just have an event hook:

  "postInstall": "rush install-autoinstaller --all"

(2) Another option would be if we think this is a reasonable boolean choice, maybe a configuration entry somewhere could be added -- alwaysInstallAutoinstallers? Setting this setting to true in rush.json would basically make rush update and rush install always include the new --autoinstaller flag, and then no event hook is necessary.

Any thoughts @octogonz ?

@isc30
Copy link
Contributor

isc30 commented Nov 29, 2022

Yeah, my case was for prettier specifically since I wanted it to run on file save from vscode. People that never did a commit (that downloaded and executed prettier) wouldn't get that working so my take was to force install it during normal installation.

I think rush install --autoinstallers will do for that, so I can replace my custom JS code with that

@elliot-nelson
Copy link
Collaborator Author

@isc30 The new option rush install --autoinstallers would install stuff if people ran it, which you could suggest (if VSCode wasn't working). But if you add it as a post install hook I think you'll end up in a recursive loop -- that's why we might need that extra command.

@isc30
Copy link
Contributor

isc30 commented Nov 30, 2022

Yeah I can change our onboarding docs to run rush install --autoinstallers before working with the repo.

In the past we couldn't do that as there wasn't a way to install autoinstallers from CLI

@iclanton iclanton added this to In Progress in Bug Triage Nov 30, 2022
@@ -153,6 +159,14 @@ export abstract class BaseInstallAction extends BaseRushAction {
try {
await installManager.doInstallAsync();

if (this._autoinstallersParameter.value) {
if (installManagerOptions.allowShrinkwrapUpdates) {
Autoinstaller.updateAutoinstallers(this.rushConfiguration);
Copy link
Member

Choose a reason for hiding this comment

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

Sorta odd that this one is sync.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm... I'll see what happens if I make it async, might be worth it it to safeguard against future need for async methods.

return;
}

console.log(colors.green(`\n${autoinstallerNames.length} autoinstaller(s) prepared.`));
Copy link
Member

Choose a reason for hiding this comment

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

What does "prepare" mean in this context? I'm not sure a user is going to understand that.

Comment on lines +313 to +316
const autoinstaller: Autoinstaller = new Autoinstaller({
autoinstallerName,
rushConfiguration
});
Copy link
Member

Choose a reason for hiding this comment

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

Will this error if the named autoinstaller doesn't exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes -- it treats it essentially like a series of steps, for example:

rush update-autoinstaller --name good1 --name good2 --name oops

Assuming oops is a misspelled autoinstaller name, the command would update good1, good2, and then dump out with the typical error. The user could then re-run the command (fixing the mistake), and it would end up printing: 2 autoinstallers skipped, 1 autoinstaller updated (since good1 and good2 had just been updated by the last run).

It would probably be a smidge nicer of an experience if we completely wrapped up (skips, successes, failures) in a bow and presented them, but failing install on an autoinstaller is unusual enough that it seemed unnecessary for this PR.

@Faithfinder
Copy link
Contributor

May I suggest rush add -m --autoinstallers?
My case is for Prettier. We have Prettier installed in most projects simply for VScode's extension discovery benefit, so upgrading it in one command would be nice

@elliot-nelson
Copy link
Collaborator Author

May I suggest rush add -m --autoinstallers? My case is for Prettier. We have Prettier installed in most projects simply for VScode's extension discovery benefit, so upgrading it in one command would be nice

@Faithfinder Ah so, rush add -m being used to upgrade a dependency (possibly), and the --autoinstallers flag means "if this is in any of my autoinstallers, update it there too".

I'm beginning to wonder if "--autoinstallers" is too brief/vague and it should be "--include-autoinstallers", to make it clear that the command in each situation does what it normally does, just also does it for autoinstallers.

@Faithfinder
Copy link
Contributor

Faithfinder commented Dec 4, 2022

May I suggest rush add -m --autoinstallers? My case is for Prettier. We have Prettier installed in most projects simply for VScode's extension discovery benefit, so upgrading it in one command would be nice

@Faithfinder Ah so, rush add -m being used to upgrade a dependency (possibly), and the --autoinstallers flag means "if this is in any of my autoinstallers, update it there too".

I'm beginning to wonder if "--autoinstallers" is too brief/vague and it should be "--include-autoinstallers", to make it clear that the command in each situation does what it normally does, just also does it for autoinstallers.

Yep!
An yes, --include-autoinstallers is indeed more clear, but I don't care too much because there's no way I'm keeping it in my head anyway - will need to check the docs most of the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Bug Triage
  
In Progress
Development

Successfully merging this pull request may close these issues.

[rush] Feature: a command to install an autoinstaller
4 participants