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

fix(docs): title match content #2574

Merged
merged 1 commit into from
Jan 4, 2022
Merged

Conversation

owl-from-hogvarts
Copy link
Contributor

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

Fix issue #2573

Copy link
Member

@rvagg rvagg left a 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:

  1. 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.
  2. 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.

@cclauss
Copy link
Contributor

cclauss commented Dec 16, 2021

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.

@owl-from-hogvarts
Copy link
Contributor Author

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.

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Dec 17, 2021

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...

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Dec 17, 2021

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 Updating-npm-bundled-node-gyp.md:

**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.

@owl-from-hogvarts
Copy link
Contributor Author

@DeeDeeG i have reviewed your changes. As i can see, they satisfy what @rvagg told about. I can approve them. But it would be cool to land them into separate PR on top of this, as you suggest

@owl-from-hogvarts
Copy link
Contributor Author

I can also propose some changes to "Force". Check out owl-from-hogvarts:docs/clarify

Copy link
Member

@rvagg rvagg left a 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!

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jan 4, 2022

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 fix(docs) would make it show under the "Bug Fixes" heading in the Changelog, as an entry_ "docs: title match content". I think the fix(): prefix is superfluous and docs: could be used as the prefix instead, so it shows under the "Documentation" heading in the Changelog. But I don't really mind either way, people can figure it out.

@rvagg rvagg merged commit 6e8f93b into nodejs:master Jan 4, 2022
@rvagg
Copy link
Member

rvagg commented Jan 4, 2022

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.

@owl-from-hogvarts owl-from-hogvarts deleted the fix/docs branch January 5, 2022 15:03
@DeeDeeG
Copy link
Contributor

DeeDeeG commented Jan 6, 2022

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.)

@AlexByte
Copy link

AlexByte commented Feb 2, 2022

just update module gulp-sass to the latest version, add sass module to the package.json and write

const sass = require('gulp-sass')(require('sass'));

to the gulpfile.js to get rid off node-gyp.

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.

5 participants