FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

meta: update NODE_MODULE_VERSION to 51 by MylesBorins · Pull Request #8808 · nodejs/node · GitHub

/ node Public

meta: update NODE_MODULE_VERSION to 51 - #8808

Merged
MylesBorins merged 1 commit into
nodejs:masterfrom
MylesBorins:node-modules-49
Sep 29, 2016
ConversationCommits (1)ChecksFiles changed
Merged

meta: update NODE_MODULE_VERSION to 51#8808
MylesBorins merged 1 commit into
nodejs:masterfrom
MylesBorins:node-modules-49

Conversation

MylesBorins commented Sep 27, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor
Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

Description of change
src: update NODE_MODULE_VERSION to 51

When V8 was updated on master to 5.4 there were ABI breaking changes.
In the past we have not landed these types of changes before a release,
and as such have only bumped the NODE_MODULE_VERSION number in the
release commit.

Since we are going to be keeping the V8 5.4 beta on master and in the
v7 betas I think it makes sense for us to bump the module number prior
to a release commit being made. It is possible that this commit should
be reverted prior to v7.0.0 being cut. Alternatively we may want to
modify our release process for V8 to include a NODE_MODULE_VERSION
bump before landing on master when applicable.

NODE_MODULE_VERSION is being bumped to 51 instead of 49 to avoid
conflicts with NODE_MODULE_VERSIONs being used in electron.

Ref: https://github.com/electron/electron/issues/5851#issuecomment-246920775
Ref: https://github.com/nodejs/node/pull/8317

/cc @nodejs/v8 @nodejs/ctc

MylesBorins added module Issues and PRs related to the module subsystem. v8 engine Issues and PRs related to the V8 dependency. meta Issues and PRs related to the general management of the project. lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 27, 2016
MylesBorins added this to the 7.0.0 milestone Sep 27, 2016

bnoordhuis commented Sep 27, 2016

Copy link
Copy Markdown
Member

You probably didn't mean to check in a.patch.

MylesBorins commented Sep 27, 2016

Copy link
Copy Markdown
Contributor Author

thanks for the quick check there. sorry about that

ofrobots commented Sep 27, 2016

Copy link
Copy Markdown
Contributor

As per discussion here: electron/electron#5851 (comment) it would be good to skip to module version 51 for better compatibility with electron.
/cc @addaleax

MylesBorins changed the title meta: update NODE_MODULE_VERSION to 49 meta: update NODE_MODULE_VERSION to 51 Sep 27, 2016

MylesBorins commented Sep 27, 2016

Copy link
Copy Markdown
Contributor Author

@ofrobots updated with suggestion

MylesBorins mentioned this pull request Sep 27, 2016
Closed
bnoordhuis approved these changes Sep 27, 2016
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM but can you explain in the commit log why it jumps from 48 to 51?

MylesBorins commented Sep 27, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

updated

I've added the following to the commit message

NODE_MODULE_VERSION is being bumped to 51 instead of 49 to avoid
conflicts with NODE_MODULE_VERSIONs being used in electron.

Ref: https://github.com/electron/electron/issues/5851#issuecomment-246920775

Fishrock123 commented Sep 27, 2016

Copy link
Copy Markdown
Contributor

cc @rvagg

addaleax approved these changes Sep 27, 2016
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with the tiny nit that I would prefer to have “#8317” written out as a full URL in the commit message.

addaleax added addons Issues and PRs related to native addons. and removed module Issues and PRs related to the module subsystem. labels Sep 27, 2016

jasnell commented Sep 27, 2016

Copy link
Copy Markdown
Member

LGTM

1 similar comment

evanlucas commented Sep 27, 2016

Copy link
Copy Markdown
Contributor

LGTM

targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 28, 2016

targos commented Sep 28, 2016

Copy link
Copy Markdown
Member

LGTM

MylesBorins commented Sep 28, 2016

Copy link
Copy Markdown
Contributor Author

I've addressed all nits. I would like to land this today if people don't mind as it is blocking smoke testing of v7 and of master.

@Fishrock123 I know you pinged @rvagg who was likely too busy with the release yesterday to take a look. Was that just for oversight or do you think he would take issue with this?

Fishrock123 commented Sep 28, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

I wouldn't mind his input here, he originally lead a lot of these bumps and it's be nice to get his thoughts. I don't think there will be objections, but ¯_(ツ)_/¯

MylesBorins commented Sep 28, 2016

Copy link
Copy Markdown
Contributor Author

I have no problem waiting then. In fact let's put it on the CTC agenda. I think it is worth discussing if we should be skipping NODE_MODULE_VERSION and if we want to change our process.

MylesBorins mentioned this pull request Sep 28, 2016
Closed
MylesBorins force-pushed the node-modules-49 branch 2 times, most recently from 0b1ce8f to 78b9d67 Compare September 29, 2016 15:49
src: update NODE_MODULE_VERSION to 51
b5bdff8 
When V8 was updated on master to 5.4 there were ABI breaking changes.
In the past we have not landed these types of changes before a release,
and as such have only bumped the NODE_MODULE_VERSION number in the
release commit.

Since we are going to be keeping the V8 5.4 beta on master and in the
v7 betas I think it makes sense for us to bump the module number prior
to a release commit being made. It is possible that this commit should
be reverted prior to v7.0.0 being cut. Alternatively we may want to
modify our release process for V8 to include a NODE_MODULE_VERSION
bump before landing on master when applicable.

NODE_MODULE_VERSION is being bumped to 51 instead of 49 to avoid
conflicts with NODE_MODULE_VERSIONs being used in electron.

Ref: electron/electron#5851 (comment)
Ref: nodejs#8317

PR-URL: nodejs#8808
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins merged commit b5bdff8 into nodejs:master Sep 29, 2016
jasnell pushed a commit that referenced this pull request Sep 29, 2016
src: update NODE_MODULE_VERSION to 51
96933df 
When V8 was updated on master to 5.4 there were ABI breaking changes.
In the past we have not landed these types of changes before a release,
and as such have only bumped the NODE_MODULE_VERSION number in the
release commit.

Since we are going to be keeping the V8 5.4 beta on master and in the
v7 betas I think it makes sense for us to bump the module number prior
to a release commit being made. It is possible that this commit should
be reverted prior to v7.0.0 being cut. Alternatively we may want to
modify our release process for V8 to include a NODE_MODULE_VERSION
bump before landing on master when applicable.

NODE_MODULE_VERSION is being bumped to 51 instead of 49 to avoid
conflicts with NODE_MODULE_VERSIONs being used in electron.

Ref: electron/electron#5851 (comment)
Ref: #8317

PR-URL: #8808
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
ruimarinho mentioned this pull request Oct 6, 2016
Closed
jasnell mentioned this pull request Oct 14, 2016
Closed
Trott removed the ctc-agenda label Oct 20, 2016
jasnell added a commit to jasnell/node that referenced this pull request Oct 24, 2016
2016-10-25, Version 7.0.0 (Current)
c1e20df 
Notable Changes:

* Buffer
  * Passing invalid input to Buffer.byteLength will now throw an error [nodejs#8946](nodejs#8946).
  * Calling Buffer without new is now deprecated and will emit a process warning [nodejs#8169](nodejs#8169).
  * Passing a negative number to allocUnsafe will now throw an error [nodejs#7079](nodejs#7079).
* Child Process
  * The fork and execFile methods now have stronger argument validation [nodejs#7399](nodejs#7399).
* Cluster
  * The worker.suicide method is deprecated and will emit a process warning [nodejs#3747](nodejs#3747).
* Deps
  * V8 has been updated to 5.4.500.36 [nodejs#8317](nodejs#8317), [nodejs#8852](nodejs#8852), [nodejs#9253](nodejs#9253).
  * NODE_MODULE_VERSION has been updated to 51 [nodejs#8808](nodejs#8808).
* File System
  * A process warning is emitted if a callback is not passed to async file system methods [nodejs#7897](nodejs#7897).
* Intl
  * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [nodejs#8908](nodejs#8908).
* Promises
  * Unhandled Promise rejections have been deprecated and will emit a process warning [nodejs#8217](nodejs#8217).
* Punycode
  * The `punycode` module has been deprecated [nodejs#7941](nodejs#7941).
* URL
  * An Experimental WHATWG URL Parser has been introduced [nodejs#7448](nodejs#7448).
jasnell added a commit that referenced this pull request Oct 25, 2016
2016-10-25, Version 7.0.0 (Current)
362fe01 
Notable Changes:

* Buffer
  * Passing invalid input to Buffer.byteLength will now throw an error [#8946](#8946).
  * Calling Buffer without new is now deprecated and will emit a process warning [#8169](#8169).
  * Passing a negative number to allocUnsafe will now throw an error [#7079](#7079).
* Child Process
  * The fork and execFile methods now have stronger argument validation [#7399](#7399).
* Cluster
  * The worker.suicide method is deprecated and will emit a process warning [#3747](#3747).
* Deps
  * V8 has been updated to 5.4.500.36 [#8317](#8317), [#8852](#8852), [#9253](#9253).
  * NODE_MODULE_VERSION has been updated to 51 [#8808](#8808).
* File System
  * A process warning is emitted if a callback is not passed to async file system methods [#7897](#7897).
* Intl
  * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [#8908](#8908).
* Promises
  * Unhandled Promise rejections have been deprecated and will emit a process warning [#8217](#8217).
* Punycode
  * The `punycode` module has been deprecated [#7941](#7941).
* URL
  * An Experimental WHATWG URL Parser has been introduced [#7448](#7448).

PR-URL: #9099
jasnell added a commit that referenced this pull request Oct 25, 2016
2016-10-25, Version 7.0.0 (Current)
1e4fafc 
Notable Changes:

* Buffer
  * Passing invalid input to Buffer.byteLength will now throw an error [#8946](#8946).
  * Calling Buffer without new is now deprecated and will emit a process warning [#8169](#8169).
  * Passing a negative number to allocUnsafe will now throw an error [#7079](#7079).
* Child Process
  * The fork and execFile methods now have stronger argument validation [#7399](#7399).
* Cluster
  * The worker.suicide method is deprecated and will emit a process warning [#3747](#3747).
* Deps
  * V8 has been updated to 5.4.500.36 [#8317](#8317), [#8852](#8852), [#9253](#9253).
  * NODE_MODULE_VERSION has been updated to 51 [#8808](#8808).
* File System
  * A process warning is emitted if a callback is not passed to async file system methods [#7897](#7897).
* Intl
  * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [#8908](#8908).
* Promises
  * Unhandled Promise rejections have been deprecated and will emit a process warning [#8217](#8217).
* Punycode
  * The `punycode` module has been deprecated [#7941](#7941).
* URL
  * An Experimental WHATWG URL Parser has been introduced [#7448](#7448).

PR-URL: #9099
imyller added a commit to imyller/meta-nodejs that referenced this pull request Oct 25, 2016
2016-10-25, Version 7.0.0 (Current)
7aae498 
    Notable Changes:

    * Buffer
      * Passing invalid input to Buffer.byteLength will now throw an error [#8946](nodejs/node#8946).
      * Calling Buffer without new is now deprecated and will emit a process warning [#8169](nodejs/node#8169).
      * Passing a negative number to allocUnsafe will now throw an error [#7079](nodejs/node#7079).
    * Child Process
      * The fork and execFile methods now have stronger argument validation [#7399](nodejs/node#7399).
    * Cluster
      * The worker.suicide method is deprecated and will emit a process warning [#3747](nodejs/node#3747).
    * Deps
      * V8 has been updated to 5.4.500.36 [#8317](nodejs/node#8317), [#8852](nodejs/node#8852), [#9253](nodejs/node#9253).
      * NODE_MODULE_VERSION has been updated to 51 [#8808](nodejs/node#8808).
    * File System
      * A process warning is emitted if a callback is not passed to async file system methods [#7897](nodejs/node#7897).
    * Intl
      * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [#8908](nodejs/node#8908).
    * Promises
      * Unhandled Promise rejections have been deprecated and will emit a process warning [#8217](nodejs/node#8217).
    * Punycode
      * The `punycode` module has been deprecated [#7941](nodejs/node#7941).
    * URL
      * An Experimental WHATWG URL Parser has been introduced [#7448](nodejs/node#7448).

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
MylesBorins deleted the node-modules-49 branch November 14, 2017 17:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
addaleax addaleax approved these changes
+2 more reviewers
evanlucas evanlucas approved these changes
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

addons Issues and PRs related to native addons. lib / src Issues and PRs related to general changes in the lib or src directory. meta Issues and PRs related to the general management of the project. semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL