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

docs: Add notes/disclaimers for upgrading the copy of node-gyp that npm uses #14

Closed
wants to merge 6 commits into from

Conversation

DeeDeeG
Copy link

@DeeDeeG DeeDeeG commented Dec 21, 2021

Checklist
Description of change

For nodejs#2574. (If merged, these changes would become part of that PR, over at the upstream repo.)

This contribution is to add some notes/warnings/disclaimers about which versions of npm these pages of documentation will work with. And a little warning that one of the methods is wiped out if you reinstall or upgrade npm or node.

My attempt to address this review comment: nodejs#2574 (review) (from the original PR you posted at the upstream repo).

(Thanks for working on this! I don't want npm 7 and 8 users left confused or without a working solution. And good docs are super important to me, personally.)

owl-from-hogvarts and others added 3 commits December 15, 2021 18:32
Also mention that this is tested on npm 8, now that npm 8 exists.
(Npm 8 is the same as npm 7 for purposes of these instructions.)
@owl-from-hogvarts
Copy link
Owner

@DeeDeeG since nodejs#2574 is merged, i can accept these changes and make PR to main repo for you

@owl-from-hogvarts owl-from-hogvarts deleted the branch owl-from-hogvarts:docs/clarify January 5, 2022 15:03
@owl-from-hogvarts owl-from-hogvarts changed the base branch from fix/docs to docs/clarify January 5, 2022 15:08
Also mention that this is tested on npm 8, now that npm 8 exists.
(Npm 8 is the same as npm 7 for purposes of these instructions.)
@owl-from-hogvarts
Copy link
Owner

owl-from-hogvarts commented Jan 5, 2022

Ops! I am not so familiar on how github PR merge works, so it seems that i mess up something. Can you rebase your changes onto docs/clarify, drop merge commit, remove duplicated commits and force push these changes please?

@owl-from-hogvarts owl-from-hogvarts changed the base branch from docs/clarify to master January 5, 2022 15:58
@owl-from-hogvarts owl-from-hogvarts changed the base branch from master to docs/clarify January 5, 2022 15:58
@DeeDeeG
Copy link
Author

DeeDeeG commented Jan 6, 2022

Hi @owl-from-hogvarts, I will make my own PR at upstream. (nodejs#2585).

There should not be merge conflicts since we are modifying different parts of the files. Just be sure to base your branch off upstream master branch, and use the same line endings (LF vs CR/LF) as upstream master has and it should be fine.

Thanks for working on this, by the way! Good docs are super important IMO.

@DeeDeeG DeeDeeG closed this Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants