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: Bugifx update all package repository fields #15477

Merged
merged 3 commits into from
Jul 11, 2019
Merged

chore: Bugifx update all package repository fields #15477

merged 3 commits into from
Jul 11, 2019

Conversation

Graham42
Copy link
Contributor

@Graham42 Graham42 commented Jul 7, 2019

Description

Looking at some gatsby packages on npm they are missing any link to the github repo source so it was difficult to find where the source code was.

This PR fixes all the repository fields for packages and add a script to be able to do this easily again in the future. Or if we want to make a sweeping change can update the script instead of manually updating each package.

Something I'm not sure about: I had to update the lerna config to include the themes directory. I think this should be ok but not sure what else this might affect.

Related Issues

Related: #12755

@Graham42 Graham42 requested review from a team as code owners July 7, 2019 01:42
@Graham42
Copy link
Contributor Author

Graham42 commented Jul 8, 2019

bootstrap is failing because the check-versions script is failing

$ babel-node scripts/check-versions.js
gatsby-starter-blog-theme:
  Depends on "gatsby-theme-blog@^1.0.0"
  instead of "[email protected]".

gatsby-starter-notes-theme:
  Depends on "gatsby-theme-notes@^1.0.0"
  instead of "[email protected]".

gatsby-starter-theme:
  Depends on "gatsby-theme-blog@^1.0.0"
  instead of "[email protected]".

  Depends on "gatsby-theme-notes@^1.0.0"
  instead of "[email protected]".

It appears there are 1.0.0 versions of some packages published but that doesn't match the version in the source?

@pieh
Copy link
Contributor

pieh commented Jul 8, 2019

bootstrap is failing because the check-versions script is failing

Those are caused by changes you did to package.json -> workspaces and fact that we didn't update deps in themes/* - this is change that we will likely make, but it affect how themes and starters are handled, so we are not confident in making this change yet

@Graham42
Copy link
Contributor Author

Graham42 commented Jul 8, 2019

Ok, I've dropped the commit that updated the package json worskpaces field.

@wardpeet
Copy link
Contributor

wardpeet commented Jul 9, 2019

Thank you!

I'm wondering why the following syntax is not correct?

"repository": {
  "type": "git",
  "url": "https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-codemods"
}

Official npm docs say it's okay https://docs.npmjs.com/files/package.json#repository

@wardpeet wardpeet added the status: awaiting author response Additional information has been requested from the author label Jul 9, 2019
@Graham42
Copy link
Contributor Author

Graham42 commented Jul 9, 2019

Yes that would also be fine, I chose the style that most commonly already exists in this repo. I don't think it makes any significant difference.

@Graham42
Copy link
Contributor Author

Made a tweak to the script to not insert repository as the last key in package.json which makes merge conflicts more likely, and rebased on the master branch.

@wardpeet
Copy link
Contributor

npm has support for monorepos now so I would suggest moving to this syntax

"repository": {
  "type" : "git",
  "url" : "https://github.com/gatsbyjs/gatsby.git",
  "directory": "packages/gatsby"
}

Sorry for the back and forth

@Graham42
Copy link
Contributor Author

I tried that syntax and it doesn't work for the npm page link to GitHub. Until it does, I would recommend the current format. See my comment here: ea1d7d1#diff-5f64c3c255be83cf700e83e922506fa3R22

@wardpeet
Copy link
Contributor

wardpeet commented Jul 10, 2019

Thank you! I didn't go through the code yet 🙊. PR looks great! I'm loving the node script so we can change all our packages at any time 🤗

@moonmeister
Copy link
Contributor

So I've noticed that most of the change logs are broken. I think this is due to Lerna prefixing urls with the "repository" url. This means "tree/master/packages/..." get's added to every link to issues, prs, commits, etc...and breaks the url. I'm guessing the new format (#15477 (comment)) mentioned by @wardpeet would fix this, but I get it may have other drawbacks.

@moonmeister
Copy link
Contributor

moonmeister commented Jul 11, 2019

Okay, did some more reading, I now have some thoughts:

@Graham42 How does the monorepo syntax not work for the nom page link? (my assumption is you mean it doesn't link you to the sub-directory) The more I think about this the repository link should be the root...and not include the folder path. NPM makes it clear that the 'repository" field is not for human consumption but for computers.

The URL should be a publicly available (perhaps read-only) url that can be handed directly to a VCS program without any modification. It should not be a url to an html project page that you put in your browser. It’s for computers.

The "homepage" field already exists in the package.json to get the user into the sub-directory(maybe your script can make sure all packages have this field). I think the repository url should be the new syntax.

"repository": {
  "type" : "git",
  "url" : "https://github.com/gatsbyjs/gatsby.git",
  "directory": "packages/gatsby"
}

Let me know if you have any questions. The full docs are here: https://docs.npmjs.com/files/package.json

@Graham42
Copy link
Contributor Author

Correct, I was meaning the sub-directory.

I missed the homepage field, that looks like a better fit, thanks! I'll update the script.

Also run as a check during linting to prevent future drift.
This commit can be recreated with "npm run check-repo-fields -- --fix"
Until these are included in the package.json workspaces field, the
update-repo-fields script wont cover them.
@Graham42
Copy link
Contributor Author

Updated script to use the monorepo format, and to also update the homepage field.

After having to update this PR multiple times I've realize that the repo would easily drift after this fix so I've refactored the script a bit to work as a check that runs as part of the circle ci lint stage.

Not sure if this might also fix #12755 now?

@moonmeister
Copy link
Contributor

Yes, it would close #12755

@wardpeet wardpeet added status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response and removed status: awaiting author response Additional information has been requested from the author labels Jul 11, 2019
@wardpeet wardpeet self-assigned this Jul 11, 2019
@wardpeet wardpeet changed the title Bugifx update all package repository fields chore: Bugifx update all package repository fields Jul 11, 2019
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Let's merge and see what it does 😄

Thanks for fixing! 🏅

@wardpeet wardpeet merged commit d6d18e2 into gatsbyjs:master Jul 11, 2019
@gatsbot
Copy link

gatsbot bot commented Jul 11, 2019

Holy buckets, @Graham42 — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. We’ve got Gatsby t-shirts, stickers, hats, scrunchies, and much more. (You can also unlock even more free swag with 5 contributions — wink wink nudge nudge.) See gatsby.dev/swag for details.
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@Graham42 Graham42 deleted the bugifx/update-repo-fields branch July 11, 2019 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants