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

[Large] Add support for inheriting platforms (monorepos) #755

Merged
merged 13 commits into from
Jun 9, 2020

Conversation

charlespierce
Copy link
Contributor

@charlespierce charlespierce commented May 29, 2020

Closes #378
Implements volta-cli/rfcs#43

Info

  • Add support for workspaces / monorepos via an extends key in package.json Volta settings (see the above-linked RFC for more information).

Changes

  • Added chain-map crate, an abstraction to allow searching a chain of HashMaps without having to collapse all of them into a single map.
  • Moved manifest parsing logic into a child module of project, instead of a separate manifest module.
  • Updated Project type to support pulling data from multiple sources by following a chain of extends directives.
    • Dependencies are loaded into a ChainMap, as we only need to search through them to determine if a dependency is there or not, we don't need to iterate / process the entire list.
    • Platform is built up as a partial (which supports temporarily not having a Node version), then converted to a PlatformSpec once all files are loaded. This will throw a more helpful error when there isn't a Node version (previously it would be an obaque parsing error because we required node to be set if volta was present).
    • A list of all files loaded is maintained, so that we can iterate through it when searching for hooks or binaries.
    • If the chain results in a cycle (e.g. file A says it extends file B, and file B says it extends file A), that is detected and an error is shown.
  • Updated the logic for writing changes to package.json to avoid clobbering any extends settings.
  • Removed unused / unmainted manifest/tests.rs file.

Tested

  • Added unit and acceptance tests to confirm that the environment is correctly detected when following the extends chain.
  • Manual testing of workspace environments to confirm that extends works as expected.

Notes

  • To maintain consistency with the rest of the code base, project.rs was moved into project/mod.rs with other submodules in project/. This has the unfortunate side effect of the diff looking like project.rs was deleted and project.mod.rs was created. While there are substantial changes, the code isn't 100% unique.
  • Similarly, the code for loading from disk was moved from manifest/mod.rs to project/serial.rs, with some modifications to support the new format. This again looks like mass removal / insertion.
  • chain-map Will likely be refactored and actually published on crates.io, so it won't have to remain a child crate permanently.

@charlespierce charlespierce marked this pull request as ready for review May 31, 2020 17:32
@charlespierce charlespierce changed the title [Large] Add support for inheriting [Large] Add support for inheriting platforms (monorepos / workspaces) Jun 1, 2020
@charlespierce charlespierce changed the title [Large] Add support for inheriting platforms (monorepos / workspaces) [Large] Add support for inheriting platforms (monorepos) Jun 1, 2020
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.

👏 nice work!

crates/chain-map/Cargo.toml Outdated Show resolved Hide resolved
crates/chain-map/src/lib.rs Outdated Show resolved Hide resolved
crates/chain-map/src/lib.rs Outdated Show resolved Hide resolved
crates/chain-map/src/lib.rs Outdated Show resolved Hide resolved
crates/volta-core/src/error/kind.rs Outdated Show resolved Hide resolved
crates/volta-core/src/project/serial.rs Outdated Show resolved Hide resolved
crates/volta-core/src/project/tests.rs Outdated Show resolved Hide resolved
crates/volta-core/src/project/tests.rs Outdated Show resolved Hide resolved
src/command/list/mod.rs Show resolved Hide resolved
src/command/which.rs Outdated Show resolved Hide resolved
@charlespierce
Copy link
Contributor Author

Published the chain-map crate separately: https://crates.io/crates/chain-map and updated this to use it as a dependency ⚡

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.

⚡ fabulous! Let's ship it!

Comment on lines +125 to +128
ExtensionCycleError {
paths: Vec<PathBuf>,
duplicate: PathBuf,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

Comment on lines +703 to +709
// Detected infinite loop in project workspace:
//
// --> /home/user/workspace/project/package.json
// /home/user/workspace/package.json
// --> /home/user/workspace/project/package.json
//
// Please ensure that project workspaces do not depend on each other.
Copy link
Contributor

Choose a reason for hiding this comment

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

More 😍!

@@ -155,6 +155,7 @@ impl<T> Default for InheritOption<T> {
}

#[derive(Clone, PartialOrd, Ord, PartialEq, Eq)]
#[cfg_attr(test, derive(Debug))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. TIL!

@chriskrycho chriskrycho merged commit 42918c1 into volta-cli:master Jun 9, 2020
@charlespierce charlespierce deleted the workspace_support branch June 9, 2020 22:34
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.

monorepo support
2 participants