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

chore(*): add directory details to the package.json of all packages #11145

Closed
wants to merge 2 commits into from
Closed

chore(*): add directory details to the package.json of all packages #11145

wants to merge 2 commits into from

Conversation

greysteil
Copy link

Specifying the directory as part of the repository field in a package.json allows npm and third party tools to provide better support when working with monorepos. For example, it allows them to correctly construct a commit diff for a specific package.

This format was accepted by npm in npm/rfcs#19.

@wardpeet
Copy link
Contributor

Hey @greysteil. I'm guessing this is not backwards compatible with yarn & older npm versions? If so we're not able to merge this at this time as we don't accept breaking changes.

@greysteil
Copy link
Author

@wardpeet what do you mean by older versions? I'm not sure how this change could break any yarn or npm versions, and it's already been adopted by the folks at react, blueprint and jest without any problems.

As I understand it, currently the repository links here aren't usable directly by git, so they may currently be breaking very old versions of npm and yarn (although I doubt it).

@@ -46,7 +46,11 @@
],
"license": "MIT",
"main": "lib/index.js",
"repository": "https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-cli",
Copy link
Contributor

Choose a reason for hiding this comment

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

So as @wardpeet noted, this is something we'll want to do (eventually?), but I do think this is maybe a worse experience in two scenarios:

  1. Browsing on npmjs.org and using the repository link, e.g. gatsby-cli
  2. Using npm repo <repo-name>, e.g. npm repo gatsby-cli

In both of these scenarios, the root of the repo will open, rather than the specific package.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed - until npm improves their links to support the new format properly the hack is probably a better experience.

Maybe worth leaving this open, but with a comment that support for npm/cli#140, and an implementation of the npmjs.org change (I believe that repo is private), would be awesome.

@DSchau DSchau changed the title Add directory details to the package.json of all packages chore(*): add directory details to the package.json of all packages Jan 25, 2019
@m-allanson m-allanson added the status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged label Jan 25, 2019
@m-allanson
Copy link
Contributor

As @greysteil noted, let's merge this once npm/cli#140 is in, and npmjs.com supports the new directory field.

@greysteil
Copy link
Author

Just realised that we could unblock this by updating all of the URLs in it to be the same as you currently have (and keeping the directory details too). Would you be 👍 if I make that change?

@DSchau
Copy link
Contributor

DSchau commented Feb 18, 2019

@greysteil would that actually work?

Maybe I'm misunderstanding, but given gatsby-source-filesystem (as an example), the URL would have to be https://github.com/gatsbyjs/gatsby.git and the directory would then be packages/gatsby-source-filesystem right?

But... the directory keyword wouldn't work with the existing URL, right?

Happy to be corrected here--more a clarifying question so I understand than anything else! Thanks!

@greysteil
Copy link
Author

@DSchau - depends how it's implemented. It wouldn't be that hard to split the directory out of existing URLs (we have logic to do that in Dependabot here already that I could port to npm).

Thinking about it, though, I think it probably makes more sense to work. I'm hoping to have PRs for npm and yarn in the next couple of days, so this should be mergeable as-is before to long.

@sidharthachatterjee
Copy link
Contributor

Looks like npm/cli#140 is merged in. Time to rebase this and merge it in!

@sidharthachatterjee sidharthachatterjee removed the status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged label Apr 25, 2019
Specifying the directory as part of the `repository` field in a `package.json`
allows third party tools to provide better support when working with monorepos.
For example, it allows them to correctly construct a commit diff for a specific
package.

This format was accepted by npm in npm/rfcs#19.
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Thank you for this @greysteil 🥇

@sidharthachatterjee
Copy link
Contributor

Rebased, fixed conflicts and added directory details to the missing packages as well!

@sidharthachatterjee
Copy link
Contributor

Waiting on https://npmjs.com to actually work with the new repository field

@sidharthachatterjee sidharthachatterjee added the status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged label Apr 25, 2019
@KyleAMathews
Copy link
Contributor

@sidharthachatterjee it won't hurt anything right to merge this now? It's a new field so will just be meaningless data until new NPM clients and npmjs.com support.

@sidharthachatterjee
Copy link
Contributor

@KyleAMathews Not a new field, it's the existing repository field but with a nested structure as opposed to a plain String

Current behaviour:

https://www.npmjs.com/package/gatsby-cli points to https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-cli

Behaviour after this (until https://npmjs.com updates this):
https://www.npmjs.com/package/gatsby-cli points to https://github.com/gatsbyjs/gatsby

Also will affect npm repo package-name

@KyleAMathews
Copy link
Contributor

Hmmm ok — how do we know when npmjs.com supports this?

@sidharthachatterjee
Copy link
Contributor

I guess we keep clicking on the link in https://www.npmjs.com/package/react-cache till the day it links to https://github.com/facebook/react/tree/master/packages/react-cache 😅

@freiksenet
Copy link
Contributor

Still not working on npm :D

@DSchau DSchau dismissed sidharthachatterjee’s stale review June 14, 2019 10:55

(not supported on npm, no reason to approve until it is!)

@moonmeister
Copy link
Contributor

moonmeister commented Jul 11, 2019

replaced by #15477 Update: sry for not using your work @greysteil, i think this pr just got lost in the pile and someone else submitted a new one. Thanks for your work, it was helpful in informing the discussion and the final fix.

@greysteil greysteil deleted the package-directory-details branch July 11, 2019 15:15
@greysteil
Copy link
Author

No worries - glad to have this done! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: blocked This issue/PR can't be solved at the moment and shouldn't be closed/merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants