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: update outdated package.json engine field #14165

Closed
wants to merge 3 commits into from

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

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

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)

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
PR-URL: #14165
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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
addaleax pushed a commit that referenced this pull request Jul 18, 2017
PR-URL: #14165
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
PR-URL: #14165
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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
PR-URL: #14165
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

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

@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
PR-URL: #14165
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
PR-URL: #14165
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.