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

feat(git): add support for ::subdir in #committish #46

Closed
wants to merge 1 commit into from

Conversation

sspiff
Copy link
Contributor

@sspiff sspiff commented Dec 10, 2019

More to do (docs and test, at least), but thought I'd pause for feedback:

What / Why

For git URLs, allow the specification of a subdirectory within the given repository via the hash portion of the URL using the syntax: #[COMMITTISH][::SUBDIR]. If SUBDIR is specified, the result object will have a new key gitSubdir with the value /SUBDIR, which can then be consumed by e.g. pacote (see References below for corresponding pacote changes). The result object's gitCommittish and gitRange keys are unchanged from current behavior.

This feature will help npm provide improved support for "monorepos" or any git repository in which the package.json is not at the repository root.

References

@darcyclarke darcyclarke added this to the OSS - Sprint 1 milestone Dec 17, 2019
@mikemimik mikemimik added Needs Tests requires tests before merging semver:minor new backwards-compatible feature labels Dec 19, 2019
@mikemimik
Copy link

Hey @sspiff just wanted to let you know that we're taking a look at this :D

@sspiff
Copy link
Contributor Author

sspiff commented Dec 19, 2019

Thanks for the update!

Note that I'm not wedded to the ::SUBDIR syntax; it was suggested previously by the community and it seemed the path of least resistance in implementation (albeit at the expense of overloading #COMMITTISH). I did investigate using query parameters as well as just extending the path, but these routes would (I think) also require changes to hosted-git-info and it wasn't clear to me that they would fit the intended scope of that module.

@mikemimik mikemimik added the Agenda will be discussed at the Open RFC call label Jan 6, 2020
@mikemimik
Copy link

Added the Agenda label. We're going to chat about this in our Open RFC call tomorrow. I'll link the issue when it gets created today.

@mikemimik mikemimik removed their assignment Jan 14, 2020
@darcyclarke darcyclarke added the Backlog a "backlogged" item that will be tracked in a Project Board label Jan 14, 2020
@sspiff
Copy link
Contributor Author

sspiff commented Jan 27, 2020

Are these open RFC meetings something that I can/should attend? I noticed in the meeting minutes that this PR was not discussed. Just wondering if I need to join in order to have it covered? In any case, apologies for not understanding the process.

@prmichaelsen
Copy link

There's strong community support for such a feature. I know many from this thread would appreciate it: npm/npm#2974

@isaacs
Copy link
Contributor

isaacs commented Mar 4, 2020

summoning @coreyfarrell to this issue.

This is an easy one to accept. We just have to write down a spec over on https://github.com/npm/rfcs so that it's documented, and then get it implemented in this repo.

If :: is a valid character in a branch name, then that won't work. Another option I've heard suggested is # for the committish and another # for the path. (So, if you want to specify a path on the default branch, it'd be git://repo##some/path.)

EDIT: scratch that, :: is better.

$ git checkout -b 'hash##hash'
Switched to a new branch 'hash##hash'
$ git checkout -b 'paamayim::nekudotayim'
fatal: 'paamayim::nekudotayim' is not a valid branch name.

@coreyfarrell
Copy link

Colons are not valid branch names (single or double colon would work). I think a single colon makes sense as it reminds me of git show master:path/to/file.js which displays path/to/file.js from the master branch. So:
git://github.com/istanbuljs/istanbuljs#commitish:packages/istanbul-lib-instrument

Or for the default branch:
git://github.com/istanbuljs/istanbuljs#:packages/istanbul-lib-instrument

Use of # needs to be mandatory when specifying a monorepo directory to avoid ambiguity between the path to the repository and the path within the repository and to allow use of the standard new URL to parse the string (just have to split url.hash on the first ':').

@isaacs
Copy link
Contributor

isaacs commented Mar 5, 2020

I think a single colon makes sense as it reminds me of git show master:path/to/file.js which displays path/to/file.js from the master branch.

This is an amazing git trick that I just learned about now. Thanks!

Also, sold, let's do a single colon!

@coreyfarrell
Copy link

Discussed this further with @isaacs on Slack yesterday, single colon will not work due to git://github.com/user/repo#semver:v1.x syntax. The idea we came to is to support multiple name value pairs after the # and specifically support path:packages/monorepo-dir for subdir selection. Some examples of valid GIT install URIs would be:

git://github.com/user/repo#master::path:subdir
git://github.com/user/repo#path:subdir::master
git://github.com/user/repo#path:subdir
git://github.com/user/repo#semver:v1.x
git://github.com/user/repo#semver:v1.x::path:subdir
git://github.com/user/repo#path:subdir::future:ignored

So the algorithm would be to take new URL(str).hash.slice(1), split on ::. Then for each :: separated item:

  • If the item has no : then it is a commit-ish
  • If the item has : then split into name and value.
    1. If the name is semver then do semver lookup of ref or tag
    2. If the name is path then use the value as the subdir to install from.
    3. If the name is unknown then warn that the name-value pair is being ignored.

This loop should error if duplicate values are found. So specifying a commit-ish and semver:range would be an error, or specifying path twice. I lean towards NOT having a hard error if unknown values are found twice, assume some future key might support an array of values. So I think having ignore:path1::ignore:path2 would not error but would cause 2 warnings?

One comment @isaacs didn't respond to my idea of warning about items of unknown name but he did agree that a hard error for unknown names would be unnecessary. Just wanted to make sure I don't claim endorsement of the warning on his behalf.

@sspiff
Copy link
Contributor Author

sspiff commented Mar 11, 2020

I'm happy to continue work on this, but I'm not sure what the next steps should be. Should a spec be submitted to https://github.com/npm/rfcs before refining any implementation?

@prmichaelsen
Copy link

@isaacs @coreyfarrell I echo sspiff's comment. Should we submit an rfc?

@kerryj89
Copy link

This would be amazing. Any further talks outside of here about implementing this?

@darcyclarke darcyclarke added this to the OSS - Sprint 17 milestone Oct 5, 2020
@darcyclarke darcyclarke removed this from the OSS - Sprint 17 milestone Oct 5, 2020
@wottpal
Copy link

wottpal commented Jan 28, 2021

Is merging this feature still planned? This would be so nice for monorepos (e.g. Gatsby.js) ❤️.

@tilgovi
Copy link

tilgovi commented Feb 9, 2021

Might be nice to align with Yarn v2 on this: https://yarnpkg.com/features/protocols#git

@tilgovi
Copy link

tilgovi commented Feb 9, 2021

https://yarnpkg.com/features/protocols#git

No arbitrary paths, but you can express a workspace to depend on.

@settings settings bot removed the Community label Sep 21, 2021
@sspiff sspiff requested a review from a team as a code owner February 10, 2022 15:05
wraithgar added a commit that referenced this pull request Jun 21, 2022
This iterates on the spec outlined by @coreyfarrel in
#46.

For each :: separated item:
  * If the item has no : then it is a commit-ish
  * If the item has : then split into name and value.
    * If the name is semver then do semver lookup of ref or tag
    * If the name is path then use the value as the subdir to install from.
    * If the name is unknown then warn that the name-value pair is being ignored.

This loop errors if duplicate values are found.
Unknown values log a warning instead of erroring.
wraithgar added a commit that referenced this pull request Jun 21, 2022
This iterates on the spec outlined by @coreyfarrel in
#46.

For each :: separated item:
  * If the item has no : then it is a commit-ish
  * If the item has : then split into name and value.
    * If the name is semver then do semver lookup of ref or tag
    * If the name is path then use the value as the subdir to install from.
    * If the name is unknown then warn that the name-value pair is being ignored.

This loop errors if duplicate values are found.
Unknown values log a warning instead of erroring.
wraithgar added a commit that referenced this pull request Jun 22, 2022
This iterates on the spec outlined by @coreyfarrel in
#46.

For each :: separated item:
  * If the item has no : then it is a commit-ish
  * If the item has : then split into name and value.
    * If the name is semver then do semver lookup of ref or tag
    * If the name is path then use the value as the subdir to install from.
    * If the name is unknown then warn that the name-value pair is being ignored.

This loop errors if duplicate values are found.
Unknown values log a warning instead of erroring.

Co-authored-by: sspiff <[email protected]>
@wraithgar
Copy link
Member

This landed in #91

@wraithgar wraithgar closed this Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backlog a "backlogged" item that will be tracked in a Project Board Needs Tests requires tests before merging semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants