tools/doc: add files to gitignore by RichardLitt · Pull Request #17224 · nodejs/node

RichardLitt · 2017-11-22T08:46:33Z

This closes #17216 by adding some files to the gitignore which stick around after an aborted build. It is meant as a temporary fix.

Checklist

commit message follows commit guidelines

Affected core subsystem(s)

.gitignore, but related to docs.

MylesBorins

This should be 'tools/doc'

Sorry for confusion

RichardLitt · 2017-11-22T08:53:44Z

No problem at all. Changed the commit, commit message, and PR title. That work?

MylesBorins · 2017-11-22T09:10:35Z

@RichardLitt it seems like I may have not properly identified the issue

it seems that there is already a tools/doc/node_modules (I had thought I checked before opening the issue but was mistaken)

The issue appears to be extra files getting added during make test and those showing up in the git diff

attached is an image of this, best I could do as this happened on other systems

I'm not sure that this approach with the git ignore is the correct solution, sorry about that. That being said we should use this PR to find the correct solution 😄

I'll update when I can dig in a bit more. Perhaps you can get this to repro on your machine

joyeecheung · 2017-11-22T09:35:16Z

About the node_modules: I think we should ignore the tools/doc/node_modules folder, but add back (!) the subfolders/files that we checked into the source code.

As for the package-lock.json...I think we should actually commit that into the source?

Also cc @refack

joyeecheung · 2017-11-22T09:41:15Z

.gitignore

So I think this should be

tools/doc/node_modules/* !tools/doc/node_modules/.bin/marked !tools/doc/node_modules/js-yaml/index.js !tools/doc/node_modules/marked

And commit the package-lock.json into the source instead of ignoring it.

RichardLitt · 2017-11-22T10:25:29Z

I edited the commit to include @joyeecheung's suggestions. I also committed the package-lock.json file.

Should be good?

joyeecheung

LGTM but I would like @watilde @refack to take a look

refack · 2017-11-22T14:01:34Z

I'll take a look, but IMHO if everything is published to npm, removing tools/doc/node_modules and adding package-lock.json is the optimal situation. That might require changing the tests to detect an offline scenario.

refack · 2017-11-22T14:41:50Z

@RichardLitt I pushed a suggested change to this PR. Feel free to remove it.

RichardLitt · 2017-11-22T14:43:02Z

Can't think of a reason to remove it, @refack. No need for every commit in this PR to come from me.

MylesBorins

LGTM

joyeecheung · 2017-11-23T04:33:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11655/

Bamieh · 2017-11-23T10:26:21Z

There is also a change in a tracked file:

	modified:   tools/doc/node_modules/js-yaml/index.js

Ignore the `test/fixtures/failcounter.js` it is added by me 😄

refack · 2017-11-23T14:36:03Z

@Bamieh did you test with this patch? It should take care of this (actually unneeded) file modification.

Bamieh · 2017-11-23T16:18:17Z

@refack yes i did run the tests and everything is in order

On branch feat/add-files-to-gitignore
Your branch is up-to-date with 'RichardLitt/feat/add-files-to-gitignore'.
nothing to commit, working tree clean

addaleax · 2017-11-23T20:11:32Z

Tbh, I don't think we should be running npm install in the first place... this change just works around that issue it seems?

MylesBorins · 2017-11-23T23:30:03Z

@addaleax while I agree, I don't think we yet have consensus on removing npm install, and in the mean time this is creating friction with the test suite

refack · 2017-11-24T00:28:49Z

I'm thinking of landing this tomorrow morning, then start working on a new PR for further improvements.

mhdawson

LGTM

PR-URL: nodejs#17224 Fixes: nodejs#17216 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

RichardLitt · 2017-11-24T15:45:08Z

\o/ thanks! Super cool. Thank you Myles for the tweet.

On Fri, Nov 24, 2017 at 10:29 AM Refael Ackermann ***@***.***> wrote: Merged #17224 <#17224>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17224 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3loYIHTXmzcvi4qAZDIQMEfOadubcpks5s5uBhgaJpZM4QnCpa> .

-- Richard | @richlitt <https://twitter.com/richlitt> | burntfen.com <http://www.burntfen.com>

addaleax · 2017-11-26T21:03:53Z

tools/doc/package.json

+  "devDependencies": {
    "js-yaml": "^3.5.2"
  },
-  "devDependencies": {},


Why this change? js-yaml is a full dependency of the doctool

At some point it you hooked js-yaml to redirect to the copy that is in ESLint.

node/tools/doc/node_modules/js-yaml/index.js

Lines 1 to 15 in bb44626

'use strict';

// Hack to load the js-yaml module from eslint.

// No other reason than that it’s huge.

const path = require('path');

const realJSYaml = path.resolve(

__dirname, '..', '..', '..', // tools/

'eslint',

'node_modules',

'js-yaml'

);

module.exports = require(realJSYaml);

This was overwritten anytime npm i was run.
(I agree we should also stop running npm i as part of the regular flows, since tools/doc should work as is in git)

Yes, not running npm i is the fix here. It’s too bad the plans for releasing the doctool as a standalone module didn’t get anywhere so far, but semantically this is backwards :(

I agree. I'll roll this back as soon as I finish testing the install-les make target.

PR-URL: #17224 Fixes: #17216 Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

gibfahn · 2018-01-03T02:24:51Z

Had to pull this into v8.9.4 as the lack of it was breaking the macOS doc-upload build job.

nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Nov 22, 2017

MylesBorins suggested changes Nov 22, 2017

View reviewed changes

MylesBorins requested review from joyeecheung and watilde November 22, 2017 08:50

RichardLitt changed the title ~~doc: add files to gitignore~~ tools/doc: add files to gitignore Nov 22, 2017

RichardLitt force-pushed the feat/add-files-to-gitignore branch 2 times, most recently from 8791f57 to 82bb669 Compare November 22, 2017 08:53

MylesBorins mentioned this pull request Nov 22, 2017

test: add doc/node_modules and doc/package_lock.json to gitignore #17230

Closed

1 task

joyeecheung reviewed Nov 22, 2017

View reviewed changes

RichardLitt force-pushed the feat/add-files-to-gitignore branch from 82bb669 to de11311 Compare November 22, 2017 10:23

joyeecheung approved these changes Nov 22, 2017

View reviewed changes

cjihrig approved these changes Nov 22, 2017

View reviewed changes

jasnell approved these changes Nov 22, 2017

View reviewed changes

refack approved these changes Nov 22, 2017

View reviewed changes

MylesBorins approved these changes Nov 23, 2017

View reviewed changes

joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 23, 2017

vsemozhetbyt mentioned this pull request Nov 23, 2017

running make test adds git untracked files #17271

Closed

refack self-assigned this Nov 24, 2017

mhdawson approved these changes Nov 24, 2017

View reviewed changes

refack force-pushed the feat/add-files-to-gitignore branch from 5d69e4d to 887e232 Compare November 24, 2017 15:25

refack removed their assignment Nov 24, 2017

refack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 24, 2017

refack merged commit 887e232 into nodejs:master Nov 24, 2017

devsnek mentioned this pull request Nov 24, 2017

loader: refactor loader #16874

Closed

3 tasks

MylesBorins mentioned this pull request Nov 26, 2017

Add remark deps #17315

Closed

gibfahn mentioned this pull request Nov 26, 2017

Add remark dependencies #17320

Closed

addaleax reviewed Nov 26, 2017

View reviewed changes

MylesBorins mentioned this pull request Dec 12, 2017

v9.3.0 proposal #17631

Merged

gibfahn added lts-watch-v6.x labels Dec 19, 2017

gibfahn added land-on-v8.x and removed lts-watch-v8.x labels Jan 3, 2018

	'use strict';

	// Hack to load the js-yaml module from eslint.
	// No other reason than that it’s huge.

	const path = require('path');

	const realJSYaml = path.resolve(
	__dirname, '..', '..', '..', // tools/
	'eslint',
	'node_modules',
	'js-yaml'
	);

	module.exports = require(realJSYaml);

Uh oh!

Conversation

RichardLitt commented Nov 22, 2017

Checklist

Affected core subsystem(s)

Uh oh!

MylesBorins left a comment

Choose a reason for hiding this comment

Uh oh!

RichardLitt commented Nov 22, 2017

Uh oh!

MylesBorins commented Nov 22, 2017

Uh oh!

joyeecheung commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RichardLitt commented Nov 22, 2017

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Nov 22, 2017

Uh oh!

refack commented Nov 22, 2017

Uh oh!

RichardLitt commented Nov 22, 2017

Uh oh!

MylesBorins left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Nov 23, 2017

Uh oh!

Bamieh commented Nov 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ignore the test/fixtures/failcounter.js it is added by me 😄

Uh oh!

refack commented Nov 23, 2017

Uh oh!

Bamieh commented Nov 23, 2017

Uh oh!

addaleax commented Nov 23, 2017

Uh oh!

MylesBorins commented Nov 23, 2017

Uh oh!

refack commented Nov 24, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

RichardLitt commented Nov 24, 2017 via email

Uh oh!

addaleax Nov 26, 2017

Choose a reason for hiding this comment

Uh oh!

refack Nov 26, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Nov 26, 2017

Choose a reason for hiding this comment

Uh oh!

refack Nov 26, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Jan 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

joyeecheung commented Nov 22, 2017 •

edited

Loading

joyeecheung Nov 22, 2017 •

edited

Loading

Bamieh commented Nov 23, 2017 •

edited

Loading

Ignore the `test/fixtures/failcounter.js` it is added by me 😄