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] Consolidate rush update and rush update-autoinstaller #3230

Open
isc30 opened this issue Feb 11, 2022 · 7 comments
Open

[rush] Consolidate rush update and rush update-autoinstaller #3230

isc30 opened this issue Feb 11, 2022 · 7 comments

Comments

@isc30
Copy link
Contributor

isc30 commented Feb 11, 2022

Hi 😄 , would you be happy if I create a PR that makes rush update also update autoinstallers?

I've found that many devs get used to rush update and forget about the autoinstallers, so this could abstract them from knowing that autoinstallers get updated in a different way (simpler api, beginner friendly).

Please let me know what you think!

@isc30
Copy link
Contributor Author

isc30 commented Feb 11, 2022

Another improvement could also be installing the autoinstallers when doing a rush install.

In our case, we have a prettier autoinstaller configured with some plugins, and these plugins cannot be found by vscode until we do the first commit that runs rush prettier and installs them.

The case is very similar to the rush update one, devs expect everything to get installed with rush install too.


At the moment we have hacked it using an eventHook for postRushInstall that runs pnpm install on each autoinstaller folder, but it isn't ideal since there are some issues with inconsistencies between pnpm version (relies on locally installed version instead of the one that rush uses).

@D4N14L
Copy link
Member

D4N14L commented Feb 17, 2022

Hmm... The purpose of the auto-installer is generally intended to lazy-install required deps only on an as-needed basis, and reduce install time/overhead. If you required the packages to be consistently updated, you could instead make the project a member of the core repository which would allow the package to be updated when rush update is run. Does this sound like it fits your needs?

@isc30
Copy link
Contributor Author

isc30 commented Feb 18, 2022

Yeah I think that would work well for the rush install case where we don't want to install the dependencies for autoinstallers and make it lazy.

In terms of having many people working on the repo, I find that rush update could also update autoinstallers and it shouldn't affect the results in any negative way:

  • autoisntallers package.json hasn't changed = pnpm install will do nothing (fast)
  • autoinstallers have package.json changed = rush will update them and get proper lockfiles etc (time saver for dev and prevents forgetting about it)

In our case, we have converted these to be normal projects that get built/installed with rush install / rush update but maybe rush update could check if the autoinstaller package.json changed and simplify that?

thanks!

@isc30
Copy link
Contributor Author

isc30 commented Mar 2, 2022

Hmm... The purpose of the auto-installer is generally intended to lazy-install required deps only on an as-needed basis, and reduce install time/overhead. If you required the packages to be consistently updated, you could instead make the project a member of the core repository which would allow the package to be updated when rush update is run. Does this sound like it fits your needs?

Hi again, I'm running into an issue when trying to make the packages be normal packages rather than autoinstallers: I can't use them when running commands from command-line.json

@isc30
Copy link
Contributor Author

isc30 commented May 4, 2022

adding a comment here with the workaround as some people requested it:

common/autoinstallers/install.js

const execSync = require('child_process').execSync
const autoinstallers = ['rush-prettier'] // add more if needed

autoinstallers.forEach(a => {
  execSync('pnpm install', { cwd: `${__dirname}/${a}`, stdio: 'inherit' })
})

rush.json:

"eventHooks": {
  "postRushInstall": ["node common/autoinstallers/install.js"],
}

@elliot-nelson
Copy link
Collaborator

Hi @isc30, if you're still interested, I'd like to get your take on the proposed changes in PR #3789.

It's not quite the same thing because the new rush install --autoinstallers command is an optional thing (the user has to opt-in, rather than it happening automatically).

Is that sufficient for your needs, or do you really need it to happen on every rush install?

@isc30
Copy link
Contributor Author

isc30 commented Nov 29, 2022

hey @elliot-nelson the PR seems like a step in the good direction. I was thinking on getting autoinstallers updated automatically on rush update since its fast (no operation) if nothing changed there.
That way if someone forgot to update the specific autoinstaller after a dependency change, they get it automatically as rush will force you to run rush update before opening a PR (and it happens quite often in my experience when doing a bulk update for tools like eslint or typescript)

@iclanton iclanton moved this to General Discussions in Bug Triage Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: General Discussions
Development

No branches or pull requests

3 participants