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

Rename 'Plugin' & 'Config' to 'Hook' and 'HookConfig' #241

Merged
merged 15 commits into from Feb 7, 2019

Conversation

@charlespierce
Copy link
Contributor

@charlespierce charlespierce commented Jan 16, 2019

Implements the changes discussed in volta-cli/rfcs#25

Changes

  • Combined config and plugin modules into a single modules named hook.
  • Updated Inventory::fetch and Distro construction to reference the hooks and use them to resolve the URLs necessary for locating the latest version, index and specific distro download.
  • Added tests to confirm the behavior of the different hook variants (Prefix and Template).

Notes

  • The node hooks for latest and index are currently treated the same way, they download the index, which is then searched to find the appropriate version. This will probably need to be documented, or if Node has an URL we can hit in the same way as the Yarn latest-version endpoint to determine the latest version, we can migrate to that.
  • This will likely have conflicts with #234, so once that is merged I will update to resolve conflicts.
  • There is a slight issue with this and the Node Index Caching: The cache is just a single file, so there is no way currently for it to distinguish between sources. That means if the index is cached and the hook changes so that it should be downloading from somewhere else, the cache will still be used. Filed #242 to address a more robust cache.
@charlespierce
Copy link
Contributor Author

@charlespierce charlespierce commented Jan 18, 2019

Resolved Merge Conflicts

@charlespierce charlespierce requested review from dherman and mikrostew Feb 1, 2019
Copy link
Collaborator

@dherman dherman left a comment

This looks great. Just a few small suggestions.

crates/notion-core/src/distro/node.rs Outdated Show resolved Hide resolved
crates/notion-core/src/distro/yarn.rs Outdated Show resolved Hide resolved
crates/notion-core/src/hook/tool.rs Outdated Show resolved Hide resolved
crates/notion-core/src/hook/tool.rs Outdated Show resolved Hide resolved
crates/notion-core/src/hook/tool.rs Outdated Show resolved Hide resolved
crates/notion-core/src/hook/tool.rs Outdated Show resolved Hide resolved
crates/notion-core/src/hook/tool.rs Show resolved Hide resolved
crates/notion-core/src/hook/tool.rs Show resolved Hide resolved
@dherman
dherman approved these changes Feb 7, 2019
Copy link
Collaborator

@dherman dherman left a comment

Exciting work!

@dherman dherman merged commit b4aefb1 into volta-cli:master Feb 7, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@charlespierce charlespierce deleted the charlespierce:rename_config_to_hooks branch Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants