Skip to content

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

Merged
dherman merged 15 commits into
volta-cli:masterfrom
charlespierce:rename_config_to_hooks
Feb 7, 2019
Merged

Rename 'Plugin' & 'Config' to 'Hook' and 'HookConfig'#241
dherman merged 15 commits into
volta-cli:masterfrom
charlespierce:rename_config_to_hooks

Conversation

@charlespierce

@charlespierce charlespierce commented Jan 16, 2019

Copy link
Copy Markdown
Contributor

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 Tool specs and fetch/install/pin refactoring #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 Make Cache of Node Index URL Aware #242 to address a more robust cache.

@charlespierce

Copy link
Copy Markdown
Contributor Author

Resolved Merge Conflicts

@dherman dherman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks great. Just a few small suggestions.

Comment thread crates/notion-core/src/distro/node.rs Outdated
Comment thread crates/notion-core/src/distro/yarn.rs Outdated
Comment thread crates/notion-core/src/hook/tool.rs Outdated
Comment thread crates/notion-core/src/hook/tool.rs Outdated
Comment thread crates/notion-core/src/hook/tool.rs Outdated
Comment thread crates/notion-core/src/hook/tool.rs Outdated
Comment thread crates/notion-core/src/hook/tool.rs
Comment thread crates/notion-core/src/hook/tool.rs

@dherman dherman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Exciting work!

@dherman
dherman merged commit b4aefb1 into volta-cli:master Feb 7, 2019
@charlespierce
charlespierce deleted the rename_config_to_hooks branch February 7, 2019 23:08
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