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

[WIP] Update package.json files to use the repository format for monorepos #3139

Closed
wants to merge 3 commits into from

Conversation

molomby
Copy link
Member

@molomby molomby commented Jun 10, 2020

This switches all package.json files over to use the preferred "object" syntax for the repository property. This allows the packages to be correctly represented within a monorepo by specifying their directory independent to the repo path. This, in turn, fixes the issue we've been having with links to other docs breaking when the package that contains the link is published to NPM.

See for example the @keystonejs/access-control package.

The docs use the correct "relative to the repo root" URL formulation:

... please consult the [Access Control Guide](/docs/guides/access-control.md).

This works on GitHub and is correctly converted when published on the docs site, but when published to NPM it results in a broken URL of...

https://github.com/keystonejs/keystone/tree/master/packages/access-control/blob/HEAD/docs/guides/access-control.md`

This is due to the packages repository config:

  "repository": "https://github.com/keystonejs/keystone/tree/master/packages/access-control"

I'm very unsure of what else this change will effect but based on my testing, it will indeed fix the document linking problem.

molomby added 2 commits June 10, 2020 18:00
…yntax for the `repository` property (https://docs.npmjs.com/files/package.json.html#repository). This allows the packages to be correctly represented within the monorepo (using the `directory` property) which fixes issues we've been having with links to other docs breaking when the package that contains the link is published to NPM.
@changeset-bot
Copy link

changeset-bot bot commented Jun 10, 2020

🦋 Changeset is good to go

Latest commit: ef16fc3

We got this.

This PR includes changesets to release 78 packages
Name Type
@keystonejs/api-tests Patch
@keystonejs/benchmarks Patch
@keystonejs/demo-project-blog Patch
@keystonejs/demo-custom-fields Patch
@keystonejs/demo-project-meetup Patch
@keystonejs/demo-project-todo Patch
@keystonejs/access-control Patch
@keystonejs/adapter-knex Patch
@keystonejs/adapter-mongoose Patch
@keystonejs/apollo-helpers Patch
@keystonejs/app-admin-ui Patch
@keystonejs/app-graphql-playground Patch
@keystonejs/app-graphql Patch
@keystonejs/app-next Patch
@keystonejs/app-nuxt Patch
@keystonejs/app-schema-router Patch
@keystonejs/app-static Patch
@keystonejs/app-version Patch
@arch-ui/docs Patch
@arch-ui/alert Patch
@arch-ui/badge Patch
@arch-ui/button Patch
@arch-ui/card Patch
@arch-ui/color-utils Patch
@arch-ui/common Patch
@arch-ui/confirm Patch
@arch-ui/controls Patch
@arch-ui/day-picker Patch
@arch-ui/dialog Patch
@arch-ui/drawer Patch
@arch-ui/dropdown Patch
@arch-ui/fields Patch
@arch-ui/filters Patch
@arch-ui/hooks Patch
@arch-ui/icons Patch
@arch-ui/input Patch
@arch-ui/layout Patch
@arch-ui/loading Patch
@arch-ui/lozenge Patch
@arch-ui/modal-utils Patch
@arch-ui/navbar Patch
@arch-ui/options Patch
@arch-ui/pagination Patch
@arch-ui/pill Patch
@arch-ui/popout Patch
@arch-ui/select Patch
@arch-ui/theme Patch
@arch-ui/tooltip Patch
@arch-ui/typography Patch
@keystonejs/auth-passport Patch
@keystonejs/auth-password Patch
@keystonejs/build-field-types Patch
create-keystone-app Patch
@keystonejs/email Patch
@keystonejs/field-content Patch
@keystonejs/field-views-loader Patch
@keystonejs/fields-authed-relationship Patch
@keystonejs/fields-auto-increment Patch
@keystonejs/fields-datetime-utc Patch
@keystonejs/fields-markdown Patch
@keystonejs/fields-mongoid Patch
@keystonejs/fields-wysiwyg-tinymce Patch
@keystonejs/fields Patch
@keystonejs/file-adapters Patch
@keystonejs/keystone Patch
@keystonejs/list-plugins Patch
@keystonejs/logger Patch
@keystonejs/mongo-join-builder Patch
@keystonejs/oembed-adapters Patch
@keystonejs/session Patch
@keystonejs/test-utils Patch
@keystonejs/utils Patch
@keystonejs/cypress-project-access-control Patch
@keystonejs/cypress-project-basic Patch
@keystonejs/cypress-project-client-validation Patch
@keystonejs/cypress-project-login Patch
@keystonejs/cypress-project-social-login Patch
@keystonejs/website Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@emmatown
Copy link
Member

I can't see your published test package but I've seen projects that use the object version and noticed that that makes the repo link on npm link to the project root instead of the package root. I quite strongly believe that having a correct repo link which points directly to the package directory is much more important than having correct doc links on npm. (Being able to quickly and reliably find the source of a given package > having correct docs links on npm) I honestly think that we should do what Babel do and not include the docs on packages on npm and instead say "go the website".

@molomby
Copy link
Member Author

molomby commented Jun 11, 2020

I've made my test package public, sorry about that.

@mitchellhamilton: I've seen projects that use the object version and noticed that that makes the repo link on npm link to the project root instead of the package root.

Yeah it does, which is not ideal.

@mitchellhamilton: I quite strongly believe that having a correct repo link which points directly to the package directory is much more important than having correct doc links on npm.

I'm on the other side of this (would prefer links that work) but admit they're both important.

It's unfortunate that, in the package.json sense, a "correct" repo link is a link to the (root of the) repo that hosts the code (and additional config to indicate the directory) but in the npmjs.com sense, a "correct" repo link is a URL that points to the location of the package code (not really a "repo link" at all).

It's a shame npmjs.com chooses to ignore the directory portion of the repository config. According to the RFC, the intent was that this info would be used in exactly the way both of us would like -- to deep link to the package source code without requiring an invalid repository URL:

Add an optional directory field to the repository declaration a package makes in its package.json. If populated, use this directory to construct a more accurate link to the package's source code from its www.npmjs.com show page, and include it in the API response from registry.npmjs.org.

-- RFC: Monorepo subdirectory declaration

Seems the website is lagging.

@mitchellhamilton: I honestly think that we should do what Babel do and not include the docs on packages on npm and instead say "go the website".

This is actually my preference as well. I recall it came up when we were originally discussing #2041 -- we were thinking of moving the current README.md docs to a different path and replacing them with a bit of short boiler plate.

Something like this I guess?

# [Package Title]

This package is part of the [Keystone Framework](https://www.keystonejs.com/).

To learn more about this package..

* [View it's docs on the Keystone website](https://www.keystonejs.com/link/to/package/docs)
* [Dive into it's source code on GitHub](https://github.com/keystonejs/keystone/link/to/original/readme/PACKAGE.md)

This solves several problems:

  • It throws directs people to the docs website where they can explore the other docs more easily
  • It works well when viewing the dir on GitHub; ie. letting people view the package Markdown but also prompting them to visit the website (which we don't currently do)
  • It links the npm package page directly to package source code (addressing your concerns I think?)

And, given that last point, I suspect it means you'll be ok using the recommended for monorepos/more technically correct repository + directory config in the package.json?

I'd be happy to action this if you think it's a good idea.

@molomby molomby changed the title Update package.json files to use the repository format for monorepos [WIP] Update package.json files to use the repository format for monorepos Jun 11, 2020
@jesstelford
Copy link
Contributor

I like the header-in-README idea 👍

@emmatown
Copy link
Member

emmatown commented Jun 11, 2020

I strongly agree with changing the contents of the READMEs to that.

Seems the website is lagging.

¯\_(ツ)_/¯. It's been like that for quite a long time.

And, given that last point, I suspect it means you'll be ok using the recommended for monorepos/more technically correct repository + directory config in the package.json?

Why is the technically correct thing important? What I care about is providing value to users. How is repository + directory config in the package.json providing value to users over the link directly to the package?

It links the npm package page directly to package source code (addressing your concerns I think?)

I want to be able to click the button on the npm site and go directly to the source of a package.(as in any package, that will not necessarily be reliable but I would like it to be as reliable as possible)

@molomby
Copy link
Member Author

molomby commented Jun 11, 2020

@mitchellhamilton: How is repository + directory config in the package.json providing value to users over the link directly to the package?

Because it is still needed to fix the "npm linking to GitHub" problem.

Assuming we do the README.md boilerplate thing, my example above was wrong -- the link to the PACKAGE.md doc would be like this...

* [Dive into it's source code on GitHub](/packages/the-thing/PACKAGE.md)

We want the URL here to be relative to the repo root so it works on GitHub (within branches, commits, forks, etc.) and makes sense on a local FS (ie. it's obvious it's a local file). Currently the only way to have these "relative to the root" links work from NPM is to have "technically correct" repo URLs (ie. pointing to the repo root, as per this PR). So this link takes users to the correct code for the package but, as you point out, the main "Repository" button goes to the repo root (at least, until npmjs.com starts respecting the directory config as they intended).

The alternative is to have the full GitHub URL in the docs like this...

* [Dive into it's source code on GitHub](https://github.com/keystonejs/keystone/blob/master/packages/the-thing/PACKAGE.md)

This links will work from NPM regardless of the repository directive so we could keep our current deep-linking repository URLs. It does breaks all the other use docs cases though..

TL;DR: Either way it's a tradeoff. I get that having the main "Repository" button take users to the right place is important, I just don't think it's worth having full URLs in the docs since that breaks some of the docs use cases. Plus, there's still a pretty obvious link that takes users to the package source code even if the main button is slightly off.

And besides...

@mitchellhamilton: I honestly think that we should do what Babel do [...]

:trollface:

@JedWatson
Copy link
Member

FWIW I think that having the repository link to the main project, as long as it's paired with a specific Readme that explains the package is part of the Keystone monorepo and has a (fully qualified) link to its package source, is fine.

Updating the site so the package docs page is in PACKAGE.md and updating the Readme so it has the better copy for npm (I'm fine with @molomby's suggestion on that) would be critical for this PR to land imo.

Right now we're giving anybody who finds a keystone package on npm a poor experience because the copy for the website page has no context for someone looking at a package (especially true if you find @keystonejs/keystone and think it's an entry point to the project... you get out of context API docs) and I think that's more important than where the repository button links.

@JedWatson
Copy link
Member

On topic, I'd add a link to the package changelog on https://changelogs.xyz/ to the readme as well, beside the links to the docs and source.

@VinayaSathyanarayana
Copy link
Contributor

Please see: https://keepachangelog.com/en/1.0.0/

@molomby
Copy link
Member Author

molomby commented Jun 11, 2020

@jed: Updating the site so the package docs page is in PACKAGE.md and updating the Readme [..] would be critical for this PR to land imo.

Yeah, I'll do a separate PR for the docs then revisit this.

@molomby molomby closed this Jun 11, 2020
@emmatown
Copy link
Member

TL;DR: Either way it's a tradeoff. I get that having the main "Repository" button take users to the right place is important, I just don't think it's worth having full URLs in the docs since that breaks some of the docs use cases. Plus, there's still a pretty obvious link that takes users to the package source code even if the main button is slightly off.

Yeah, it's totally a tradeoff. I think that specifically for this case, because the READMEs with the "here are some links" content will only really be read by people looking at it on npm or people browsing the repo on GitHub so having it be more "correct" locally and on branches isn't super important. They'll only care about getting to the. When it's actually docs, I get that there's a big problem with having links that point to master but when it's just general links and there's no content, I don't see the problem.

@jesstelford
Copy link
Contributor

@VinayaSathyanarayana we use an awesome tool that @mitchellhamilton & @Noviny built called changesets.

It's like the third evolution form of keepachangelog.com 😉

@timleslie timleslie deleted the molomby/fix-package-json-repository-configs branch July 10, 2020 01:20
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.

Internal links in docs either work on the docs site OR GitHub (but not both)
6 participants