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

tools: don't create package-lock.json #17330

Closed
wants to merge 1 commit into from

Conversation

refack
Copy link
Contributor

@refack refack commented Nov 26, 2017

An alternative attempt to minimize possible noise from running make lint-md-build

Refs: #17320

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tools

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 26, 2017
@refack refack mentioned this pull request Nov 26, 2017
@refack
Copy link
Contributor Author

refack commented Nov 26, 2017

/CC @nodejs/build

@gibfahn
Copy link
Member

gibfahn commented Nov 26, 2017

Does this undo the benefit of adding the package-lock.json in the first place?

@refack
Copy link
Contributor Author

refack commented Nov 26, 2017

Does this undo the benefit of adding the package-lock.json in the first place?

Good question. I would think not, but I'll check.

@gibfahn
Copy link
Member

gibfahn commented Nov 26, 2017

I assumed it would due to the docs:

The --no-shrinkwrap argument, which will ignore an available package lock or shrinkwrap file and use the package.json instead.

@refack
Copy link
Contributor Author

refack commented Nov 26, 2017

@gibfahn yes, that was not the right way. From what I learned now this is what npm-shrinkwrap.json is for. It is only created if requested explicitly, and has the same validation & performance benefits.

From CI https://ci.nodejs.org/job/node-test-linter/13943/console:

18:11:36 HEAD is now at da550abe6e... tools: use shrinkwrap instead of package-lock
...
18:11:38 + gmake lint-md-build
18:11:38 Markdown linter: installing remark-cli into tools/
18:11:45 added 155 packages in 4.976s
18:11:45 Markdown linter: installing remark-preset-lint-node into tools/
18:11:48 added 48 packages in 2.29s

@@ -104,6 +104,7 @@ deps/npm/node_modules/.bin/
/SHASUMS*.txt*

# test artifacts
package-lock.json
Copy link
Member

Choose a reason for hiding this comment

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

There is also a package-lock.json in tools/doc..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack.
renamed.

@refack
Copy link
Contributor Author

refack commented Nov 28, 2017

@addaleax I've added a commit that fixes the inconsistency in tools/doc/package.json

@refack refack mentioned this pull request Nov 28, 2017
2 tasks
@addaleax
Copy link
Member

@refack I still think #17320 is the way to go here, tbh

@refack
Copy link
Contributor Author

refack commented Dec 2, 2017

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

As I commented in #17320 (comment), landing this does not preclude adding the dependencies later.

@refack refack added tools Issues and PRs related to the tools directory. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 2, 2017
@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 12, 2017
@BridgeAR
Copy link
Member

@refack I am not sure why you added the ready label even though there is no LG at all?

@addaleax do you want to make your concerns explicit?

@addaleax
Copy link
Member

@BridgeAR I’m good here… but yes, no approvers yet and I would still prefer the in-tree approach.

@BridgeAR BridgeAR requested a review from a team February 7, 2018 03:45
@BridgeAR
Copy link
Member

BridgeAR commented Feb 7, 2018

@nodejs/tsc can you please have a look to decide what to do?

@BridgeAR
Copy link
Member

@nodejs/collaborators PTAL.

@BridgeAR
Copy link
Member

@refack would you be so kind and rebase this? :-)

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR
Copy link
Member

@refack if I am not mistaken, a better alternative is #20109 and this should likely be closed?

@BridgeAR
Copy link
Member

Ping @refack

@Trott
Copy link
Member

Trott commented May 23, 2018

I'm going to remove the tsc-review label on this one. @BridgeAR added it but indicated in a comment at a later date that a better alternative (also authored by @refack) appeared. Please feel free to re-add tsc-review if appropriate.

@BridgeAR
Copy link
Member

I am closing this due to no response. @refack please feel free to reopen in case you want to continue working on this.

@BridgeAR BridgeAR closed this May 25, 2018
@refack refack reopened this Jun 1, 2018
@refack
Copy link
Contributor Author

refack commented Jun 1, 2018

Hello from jenkins

@refack
Copy link
Contributor Author

refack commented Jun 1, 2018

test this please

@ryzokuken
Copy link
Contributor

@refack what's the status of this? Are you still working on it?

@maclover7
Copy link
Contributor

ping @refack?

@maclover7 maclover7 added the wip Issues and PRs that are still a work in progress. label Aug 11, 2018
@refack
Copy link
Contributor Author

refack commented Aug 29, 2018

Made mostly obsolete by #22399 and others

@refack refack closed this Aug 29, 2018
@refack refack deleted the less-noisy-lint-md-build branch August 29, 2018 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.