Thinkmill / manypkg

☔️ An umbrella for your monorepo
MIT License
872 stars 48 forks source link

`repository` validation fails in some cases #116

Open Andarist opened 2 years ago

Andarist commented 2 years ago

For this:

{ "rootRepositoryField": "https://github.com/Thinkmill/manypkg" }

we get this back from parse-github-url:

{
  "protocol": "https:",
  "slashes": true,
  "auth": null,
  "host": "github.com",
  "port": null,
  "hostname": "github.com",
  "hash": null,
  "search": null,
  "query": null,
  "pathname": "Thinkmill/manypkg",
  "path": "Thinkmill/manypkg",
  "href": "https://github.com/Thinkmill/manypkg",
  "filepath": null,
  "owner": "Thinkmill",
  "name": "manypkg",
  "repo": "Thinkmill/manypkg",
  "branch": "master",
  "repository": "Thinkmill/manypkg"
}

As we might notice the branch has defaulted to master which in turn makes per-package repository field like https://github.com/Thinkmill/manypkg/tree/main/packages/cli incorrect and results in the error:

☔️ error @manypkg/cli has a repository field of "https://github.com/Thinkmill/manypkg/tree/main/packages/cli" when it should be "https://github.com/Thinkmill/manypkg/tree/master/packages/cli"

The quickest fix for me was to just remove the root "repository" field but I think that the situation here should be improved so nobody would face that in the future.

emmatown commented 2 years ago

We don't actually look at the branch from parse-github-url, you can set the branch used for the rule in the root package.json:

"manypkg": {
  "defaultBranch": "main"
}
Andarist commented 2 years ago

Hah, right! I've just checked that in here we get incorrect~ (in my situation) branch and I've assumed that it's the culprit.

Since most of the stuff is really convention-based and not configuration-based. Why this option does exist? Why it was not chosen to validate and fix the rootRepositoryField so it would contain a branch name explicitly and for that to be the source of this "option"? In addition to that - without rootRepositoryField the per-package packag.json#repository is not validated at all so each package could end up with highly different/broken URLs there

emmatown commented 2 years ago

What would the root repository field look like with the branch name included? afaik, there isn't a form that would include it?

Andarist commented 2 years ago

If we visit https://github.com/Thinkmill/manypkg and press Y on the keyboard we are being redirected to an URL containing the hash (eg. https://github.com/Thinkmill/manypkg/tree/5f6cdedf6843d60144c1ea65b5a8ef0c4b7f0bd5) - the URL isn't really bound to a hash though, it can take any git revision. So we can just replace this with a branch name and end up with a valid URL such as: https://github.com/Thinkmill/manypkg/tree/master

emmatown commented 2 years ago

I did not know that was a thing! Supporting that would be great

Andarist commented 2 years ago

Can't say it's anywhere on my todo list but it's great to keep this as an issue "for the better times" 😉

Andarist commented 2 years ago

TIL that repository.directory is supported:

"repository": {
  "type" : "git",
  "url" : "https://github.com/facebook/react.git",
  "directory": "packages/react-dom"
}

https://docs.npmjs.com/cli/v6/configuring-npm/package-json#repository

emmatown commented 2 years ago

Yeah though I'm not sure I've seen anything that actually uses it, e.g. the repository link for https://www.npmjs.com/package/react goes to the root of the repo which is quite annoying but the link on https://www.npmjs.com/package/@emotion/react goes to the package directory