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

Simplify repository element in npm's project.json to use https URL #1127

Merged
merged 7 commits into from
Dec 3, 2020

Conversation

jcansdale
Copy link
Contributor

Why:

The repository element in the package.json file can be dramatically simplified from:

"repository" : {
    "type" : "git",
    "url": "ssh://git@github.com/OWNER/REPOSITORY.git",
    "directory": "packages/name"
  },

..to just:

"repository" : "https://github.com/OWNER/REPOSITORY",

This will be less error prone because it can be easily copy/pasted from GitHub repository URLs.

We could simplify it further to just "repository" : "OWNER/REPOSITORY",, because npm treats GitHub as it's default Git repository host. This would break symmetry with GHES, so maybe the https form is preferable?

See here for "repository" : "OWNER/REPOSITORY", form:
https://docs.npmjs.com/cli/v6/configuring-npm/package-json#repository:~:text=%22repository%22%3A%20%22npm%2Fnpm%22

What's being changed:

Check off the following:

"repository" : {
"type" : "git",
"url": "ssh://git@{% if currentVersion == "free-pro-team@latest" %}github.com{% else %}<em>HOSTNAME</em>{% endif %}/<em>OWNER</em>/<em>REPOSITORY</em>.git",
"directory": "packages/name"
Copy link
Contributor

Choose a reason for hiding this comment

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

It explains the "directory" value in the previous paragraph

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I don't think we need to document the directory value here (it isn't used by GitHub Packages).

@toddself
Copy link

toddself commented Nov 6, 2020

This is different than how npm init configures the repository information if you have a git remote already configured, so it might potentially confusing?

[09:47:37] $ npm init -y
Wrote to /Users/toddkennedy/src/vimwiki/package.json:

{
  "name": "vimwiki",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "repository": {
    "type": "git",
    "url": "git+https://github.com/toddself/vimwiki.git"
  },
  "keywords": [],
  "author": "",
  "license": "ISC",
  "bugs": {
    "url": "https://github.com/toddself/vimwiki/issues"
  },
  "homepage": "https://github.com/toddself/vimwiki#readme"
}

Ideally maybe we should say to use npm init (-y just accepts the default answers) to generate the package.json file? Then again I hate writing json by hand so it might be just me being lazy lol

…ystem/configuring-npm-for-use-with-github-packages.md
@jcansdale
Copy link
Contributor Author

jcansdale commented Nov 6, 2020

@toddself,

This is different than how npm init configures the repository information if you have a git remote already configured, so it might potentially confusing?

Interesting, I didn't know that. I tend to npm init before git init.

I guess ssh://git@github.com/OWNER/REPOSITORY.git betrays the example was likely generated on a Mac, compared to https://github.com/OWNER/REPOSITORY which is more common for Windows users.

If I'm adding it by hand, I much prefer the "repository" : "https://github.com/OWNER/REPOSITORY", form. 🙂

@janiceilene
Copy link
Contributor

👋 @jcansdale @toddself, let me know when this is ready for a writer review!

@jcansdale
Copy link
Contributor Author

jcansdale commented Nov 9, 2020

@toddself,

This is different than how npm init configures the repository information if you have a git remote already configured, so it might potentially confusing?

Thinking more about this. If a user does npm init inside an existing repository, the repository element will automatically be added and everything will just work.

I think the following section:

https://github.com/github/docs/blob/a73479193b2287ea49fe19ab44546d0895cf0a1f/content/packages/using-github-packages-with-your-projects-ecosystem/configuring-npm-for-use-with-github-packages.md#publishing-multiple-packages-to-the-same-repository

... is most relevant for users who are adding the repository element after the fact. In this case, I think the simplicity of a copy/pastable https element makes more sense.

What do you think?

@toddself
Copy link

@jcansdale i think that makes total sense!

@jcansdale
Copy link
Contributor Author

@janiceilene I think we're ready for a writer review. 👍

@janiceilene janiceilene added content This issue or pull request belongs to the Docs Content team ecosystem This issue or pull request should be reviewed by the Docs Ecosystem team packages This issue or pull request should be reviewed by the docs packages team labels Nov 17, 2020
@github-actions
Copy link
Contributor

This PR is stale because it has been open 7 days with no activity and will be automatically closed in 3 days. To keep this PR open, update the PR by adding a comment or pushing a commit.

@github-actions github-actions bot added the stale There is no recent activity on this issue or pull request label Nov 25, 2020
@jcansdale jcansdale removed the stale There is no recent activity on this issue or pull request label Nov 25, 2020
@jcansdale
Copy link
Contributor Author

Hi @janiceilene, I've just marked this as not-stale. It's ready for a writer review. 🙂

@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2020

This PR is stale because it has been open 7 days with no activity and will be automatically closed in 3 days. To keep this PR open, update the PR by adding a comment or pushing a commit.

@github-actions github-actions bot added the stale There is no recent activity on this issue or pull request label Dec 2, 2020
@runleonarun
Copy link
Contributor

We will review this as soon as we can!

Copy link
Contributor

@runleonarun runleonarun left a comment

Choose a reason for hiding this comment

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

This looks good!

@github-actions github-actions bot removed the stale There is no recent activity on this issue or pull request label Dec 3, 2020
@janiceilene janiceilene added the ready to merge This pull request is ready to merge label Dec 3, 2020
@janiceilene janiceilene merged commit d35ed4e into main Dec 3, 2020
@janiceilene janiceilene deleted the repository-https branch December 3, 2020 23:04
@github-actions
Copy link
Contributor

github-actions bot commented Dec 3, 2020

Thanks very much for contributing! Your pull request has been merged 🎉 You should see your changes appear on the site in approximately 24 hours.

jnidzwetzki pushed a commit to jnidzwetzki/docs that referenced this pull request Oct 6, 2022
Co-authored-by: Lana Brindley <github@lanabrindley.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content This issue or pull request belongs to the Docs Content team ecosystem This issue or pull request should be reviewed by the Docs Ecosystem team packages This issue or pull request should be reviewed by the docs packages team ready to merge This pull request is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants