Skip to content

tools: update outdated package.json engine field#14165

Closed
strugee wants to merge 3 commits intonodejs:masterfrom
strugee:remove-engines-field
Closed

tools: update outdated package.json engine field#14165
strugee wants to merge 3 commits intonodejs:masterfrom
strugee:remove-engines-field

Conversation

@strugee
Copy link
Contributor

@strugee strugee commented Jul 11, 2017

This engines field clearly hasn't been true for a long time and is just useless. So get rid of it.

Checklist
Affected core subsystem(s)

tools

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Jul 11, 2017
lpinca
lpinca approved these changes Jul 11, 2017
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

I guess it is technically true but so outdated that it is no longer relevant.

@refack
Copy link
Contributor

refack commented Jul 11, 2017

Is it worth updating? (I'm deducing latest active version is https://github.com/AlastairTaft/node-doc-generator)

@strugee
Copy link
Contributor Author

strugee commented Jul 11, 2017 •
edited
Loading

I guess it is technically true but so outdated that it is no longer relevant.

Nope, it uses let and const and stuff these days.

@refack it looks to me like that's an extraction of what's in-tree here right? Not sure what you're thinking should happen here

@refack
Copy link
Contributor

refack commented Jul 11, 2017

@refack it looks to me like that's an extraction of what's in-tree here right? Not sure what you're thinking should happen here

I assumed the dependency was reversed (that https://github.com/AlastairTaft/node-doc-generator was more updated), so NM.

@gibfahn
Copy link
Member

gibfahn commented Jul 11, 2017

Nope, it uses let and const and stuff these days.

Then shouldn't it be >=4?

@strugee
Copy link
Contributor Author

strugee commented Jul 11, 2017

Then shouldn't it be >=4?

I guess? I mean, I didn't spend a lot of time looking for other ES6 features so I don't actually know. If #14164 lands it will require Array#includes which isn't in Node 4.

@gibfahn
Copy link
Member

gibfahn commented Jul 12, 2017 •
edited
Loading

I guess? I mean, I didn't spend a lot of time looking for other ES6 features so I don't actually know. If #14164 lands it will require Array#includes which isn't in Node 4.

I'd rather keep it if there's a chance that it might save someone some confusion at some point. Maybe set it to >=4 here and change it to >=6 in #14164? That'd also make it clear that that PR couldn't be backported to Node 4.x (not that that's likely to happen, but anyway).

EDIT: Or just make it >=6 here and be done with it.

@strugee
Copy link
Contributor Author

strugee commented Jul 12, 2017

@gibfahn I added a new commit changing it to >=6 - if other people like it I can squash them together (or someone can do it on merge)

gibfahn
gibfahn approved these changes Jul 12, 2017
View reviewed changes
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

LGTM (it can be squashed by you or on landing, either is fine).

@strugee strugee changed the title tools: remove outdated package.json engine field tools: update outdated package.json engine field Jul 12, 2017
jasnell pushed a commit that referenced this pull request Jul 14, 2017
@strugee @jasnell
tools: update package.json engine field
49e63ff 
PR-URL: #14165
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jul 14, 2017

Landed in 49e63ff

@jasnell jasnell closed this Jul 14, 2017
@addaleax addaleax mentioned this pull request Jul 18, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@strugee @addaleax
tools: update package.json engine field
5543802 
PR-URL: #14165
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@strugee @Fishrock123
tools: update package.json engine field
014f514 
PR-URL: #14165
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@strugee strugee deleted the remove-engines-field branch July 20, 2017 23:32
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@strugee @MylesBorins
tools: update package.json engine field
439f3da 
PR-URL: #14165
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

MylesBorins commented Aug 16, 2017

I've backported this. let me know if it should have been

@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@strugee @MylesBorins
tools: update package.json engine field
f395b48 
PR-URL: #14165
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
@strugee @MylesBorins
tools: update package.json engine field
7d851e3 
PR-URL: #14165
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

+3 more reviewers

@refack refack refack approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@gibfahn gibfahn gibfahn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.