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

[Medium] Reimplement 'volta install' using global package install #808

Merged
merged 4 commits into from
Sep 4, 2020

Conversation

charlespierce
Copy link
Contributor

Info

  • First step of the package rework: Install packages using npm install --global with a redirect into a Volta-managed directory.
  • Install into a temporary directory first, to avoid creating unnecessary directories in the event of an error.
  • This PR explicitly doesn't include parsing the package.json or writing configs & shims, those will be added in a future PR.

Changes

  • Added new error messages for new error paths.
  • Created global_install method to actually run the install command.
  • Created skeleton implementation of Tool::install for Package
  • Included TODO comments for areas that will be added in future PRs.

Tested

All tested locally as automated tests will be added in a future PR, when the entire workflow is complete

  • Confirmed packages are installed and executable in the expected location.
  • Verified that the issue described in Cannot find module when sub-package depends on the root package #762 is resolved (since the directory structure includes node_modules)
  • Ensured directories aren't created when there is an error with installation (e.g. when the package doesn't exist)

Notes

  • volta fetch <package> was made into an error because with the switch to using npm install --global directly it doesn't work the way it would be expected to.
    • Since the install process is entirely managed by npm, we can't reasonably have a "pre-fetched" version around to shortcut.
    • If we allowed it, the volta install <package> process would ultimately overwrite it anyway, because we would need to ensure that we were using the potentially new Node version.

Copy link
Contributor

@chriskrycho chriskrycho 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! I had a couple questions, but wholly for my own learning!

@charlespierce charlespierce merged commit 6432fae into volta-cli:main Sep 4, 2020
@charlespierce charlespierce deleted the package_install_global branch September 4, 2020 19:05
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.

2 participants