-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(docs): title match content #2574
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wew, those are some obscure commands you're getting into there. I think I'm OK with this but I reckon there's a couple of improvements in the "Updating" doc:
- I don't think you're explaining that this is all going to be blown out the next time npm (or note) gets installed again, so maybe just a quick note at the top section before you get to the how sections.
- I think you should also link to the "Force" doc from the "Updating" doc, because once they see what's involved they may decide that it's just easier to set the config rather than futzing with their installed copy.
My sense is that a lot of users come here frustrated and just want simple instructions that make things work again. We need a TL;DR approach that quickly tells them how to put the train back on the tracks again. After that has been done, we can have extra text on the whys and wherefores for those who are interested to read further. |
Thanks for feedback, but i cant accomplish this changes, because this PR fix exactly what @DeeDeeG told to fix: restore old content of "update" and move current content to another file. So i will not include any other fixes to this PR. If you want them, I can make a separate PR but only after merge of this. |
Note: The value of these old "complicated" docs is that they still work on npm 7 and 8. See: npm/cli#2839 Hi folks. I feel quite responsible for the state of this PR, as it follows my suggestion exactly, and restores content that I was the most recent person to seriously work on. This PR has no new content other than a filename and markdown heading. It restores one file's worth of old content, verbatim, so that npm 7 and 8 users have something that works. (That's the stuff I worked on.) And moves the revamped instructions (npm 6-compatible only) to a separate file. Again copied verbatim, other than the new filename and markdown heading at the top. Improving the content is much harder than what this PR currently does. It's just copy-pasting old content and giving the newer approach a new filename and title. Please consider whether desired improvements might be out of scope here? If the desired improvements truly block this, I am the most recent person to do a lot of work and testing on the old docs this PR restores. I can try to find time to improve them, I suppose... |
I drafted up some changes addressing @rvagg's review comment... Take a look at my changes here: DeeDeeG@af2b04b (rich diff) Reviews/nits welcome, and (not to speak prematurely) I can post these as a PR to @owl-from-hogvarts's branch to make it part of this PR if desired. Here's some notes that could be added right under the title/header on **Note: These instructions are (only) tested and known to work with npm 8 and older.**
**Note: These instructions will be undone if you reinstall or upgrade your install of npm or node! For a more permanent (and simpler) solution, see [Force-npm-to-use-global-node-gyp.md](https://github.com/nodejs/node-gyp/blob/master/docs/Force-npm-to-use-global-node-gyp.md)** I would also mention npm 8 later in the file, since the instructions work exactly the same on npm7 as on npm 8. -If your npm version is ___7___, do:
+If your npm version is ___7 or 8___, do: (Applies once in the macOS/Linux section, once in the Windows section.) If it's not too bizarre, I can post this as a PR to @owl-from-hogvarts' fork targeting this branch and it will become part of this PR. Feedback on these changes welcome. EDIT: I already see a problem. I need to mention the "simpler" solution does NOT work on npm 7 or 8. I can possibly work on this more tomorrow. |
I can also propose some changes to "Force". Check out owl-from-hogvarts:docs/clarify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I should try and be less picky about these doc changes, I'm genuinely appreciative to both of you for your contributions to these and don't want to discourage the enthusiasm!
Thanks for the review! (And to the small extent I've been a maintainer on repos that get issues/PRs posted, I have also been rather picky about it.) Regarding my proposed changes to address the review comment from before: I'll be happy to re-post those at this repo instead, in a day or two, if this indeed merges. (When I get a free moment.) (I don't think @owl-from-hogvarts is keen to merge my stuff to this branch/PR? I tried to submit the changes to this PR indirectly, via https://github.com/owl-from-hogvarts/pull/14, but that's more hassle than @owl-from-hogvarts probably meant to sign up for here... If I'm proposing changes, I should probably be accountable to feedback, not indirectly introduce them through someone else's PR. It also may not have been clear that by merging that, it would update this PR as well? I can agree as @owl-from-hogvarts already said, it would be simpler for them if I did it separately.)
Super nitpicking/maybe unimportant stuff about commit message for Changelog purposes, (click to expand, or feel free to skip):The PR title used here |
Thanks for the comments @DeeDeeG, it might be best if you just submit your PR here and we'll work on this piecemeal. This was one benefit of docs being a wiki and we probably should be a lot more relaxed about getting doc changes in, more wiki-style. |
I have posted my stuff here: #2585 (It adds some notes/disclaimers for upgrading the copy of node-gyp that npm will use. Mostly about compatibility with npm 6, vs 7/8. Note that npm 6 is still relevant as it is bundled with Node 12.x and14.x, which are LTS releases of Node. Node 14.x will be supported until 2023-04-30.) |
just update module
to the gulpfile.js to get rid off node-gyp. |
Checklist
npm install && npm test
passesDescription of change
Fix issue #2573