doc: document process.versions.modules by kzurawel · Pull Request #9901 · nodejs/node · GitHub

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc process

Description of change

This commit adds a description of process.versions.modules,
based on the comment in src/node_version.h lines 47-50.

This commit adds a description of `process.versions.modules`, based on the comment in `src/node_version.h` lines 47-50.

Thanks for the contribution. I'm not sure if we have something in the docs that explains ABI, but if we do, making ABI a link to that explanation would be great.

I think this should be more of a "In addition, the modules property indicates ...", because this version contradicts the earlier sentence, the modules property is not a version of Node.js or its dependencies.

Perhaps it would be even better by enumerating each individual property, and describing it?

There's nothing in the docs that explains ABI currently. Could be a good addition to https://nodejs.org/dist/latest-v7.x/docs/api/addons.html at some point (/cc @nodejs/documentation). I wouldn't make it a condition for landing this PR tho

@kzurawel are you still working on this?

I'm still "working" on this, but I'm not sure what needs to be done at this point.

You need to read all comments, and if they suggest a change, implement it or explain why not. If they ask a question, answer it as a comment, or answer implicitly by pushing to the PR.

Specifically, in #9901 (comment)

I said:

I think this should be more of a "In addition, the modules property indicates ...", because this version contradicts the earlier sentence, the modules property is not a version of Node.js or its dependencies.

You should do this, or something similar, or comment on why you don't think it should be done.

Perhaps it would be even better by enumerating each individual property, and describing it?

This is phrased as a question, so you can answer it, or just do what I suggeest and change the doc format, or not change the doc format if you don't have the time or don't think its a good idea.

@kzurawel Are you still working on this?

(If not, does anyone else want to take it over and get it across the finish line?)

I think that's all, I'll leave describing the other properties for future work.

@sam-github Looks good to you?

LGTM with a nit, thanks for this!

nit: This sounds like Node will load native modules built for newer modules values, which isn’t the case… maybe something like for a different fits better?

node/src/node_version.h

Lines 46 to 52 in e03ee71

    
           /** 
        
            * When this version number is changed, node.js will refuse 
        
            * to load older modules.  This should be done whenever 
        
            * an API is broken in the C++ side, including in v8 or 
        
            * other dependencies. 
        
            */ 
        
           #define NODE_MODULE_VERSION 51 /* Node.js v7.0.0 */

Documentation text mirrors the only available documentation for this property.... an in-source comment. @addaleax are you saying the comment is wrong?

If so, this PR should change both the comment and the docs.

@sam-github No, I didn’t say the current text is wrong. It’s just incomplete and might therefore be misleading.

If so, this PR should change both the comment and the docs.

No objections to that, but, like, this isn’t worth holding up the PR or anything imho.

Right, so text should be something like "Node.js will refuse to load modules that weren't compiled against its own
module ABI number."

Landed in f368eee.

I'll update the text further (based on the back-and-forth between @addaleax and @sam-github) in a subsequent PR.

This commit adds a description of `process.versions.modules`, based on the comment in `src/node_version.h` lines 47-50. PR-URL: nodejs#9901 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This commit adds a description of `process.versions.modules`, based on the comment in `src/node_version.h` lines 47-50. PR-URL: #9901 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Refs: nodejs#9901 (comment)

PR-URL: nodejs#10419 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Refs: nodejs#9901 (comment)

This commit adds a description of `process.versions.modules`, based on the comment in `src/node_version.h` lines 47-50. PR-URL: #9901 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

PR-URL: nodejs#10419 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Refs: nodejs#9901 (comment)

PR-URL: #10419 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Refs: #9901 (comment)

doc: document process.versions.modules

80f97c2

This commit adds a description of `process.versions.modules`, based on the comment in `src/node_version.h` lines 47-50.

nodejs-github-bot added doc Issues and PRs related to the documentations. process Issues and PRs related to the process subsystem. labels Dec 1, 2016

Trott reviewed Dec 1, 2016

View reviewed changes

jasnell approved these changes Dec 5, 2016

View reviewed changes

Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 6, 2016

doc: update wording for process.versions.modules

91d0465

nodejs-github-bot added the lts-watch-v6.x label Dec 21, 2016

jasnell approved these changes Dec 22, 2016

View reviewed changes

addaleax approved these changes Dec 22, 2016

View reviewed changes

Trott closed this Dec 23, 2016

Trott mentioned this pull request Dec 23, 2016

doc,src: clarify doc/comment about ABI number #10419

Closed

3 tasks

targos added land-on-v6.x and removed lts-watch-v6.x labels Dec 26, 2016

targos added lts-watch-v4.x and removed lts-watch-v4.x labels Dec 26, 2016

MylesBorins mentioned this pull request Dec 27, 2016

V6.9.3 proposal #10394

Merged

Trott added a commit to Trott/io.js that referenced this pull request Dec 28, 2016

doc: clarify information about ABI version

6755347

Refs: nodejs#9901 (comment)

evanlucas mentioned this pull request Jan 3, 2017

v7.4.0 release proposal #10589

Merged

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Jan 23, 2017

               The `process.versions` property returns an object listing the version strings of
-              Node.js and its dependencies.
+              Node.js and its dependencies. `process.versions.modules` indicates the current
+              ABI version, which is increased whenever a C++ API changes. Node.js will refuse

-              Node.js and its dependencies.
+              Node.js and its dependencies. In addition, `process.versions.modules` indicates
+              the current ABI version, which is increased whenever a C++ API changes. Node.js
+              will refuse to load native modules built for an older `modules` value.

	/**
	* When this version number is changed, node.js will refuse
	* to load older modules. This should be done whenever
	* an API is broken in the C++ side, including in v8 or
	* other dependencies.
	*/
	#define NODE_MODULE_VERSION 51 /* Node.js v7.0.0 */

doc: document process.versions.modules by kzurawel · Pull Request #9901 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: document process.versions.modules - #9901

doc: document process.versions.modules#9901kzurawel wants to merge 2 commits intonodejs:masternodejs/node:masterfrom kzurawel:process_versions_moduleskzurawel/node:process_versions_modulesCopy head branch name to clipboard

Conversation

kzurawel commented Dec 1, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Trott Dec 1, 2016

Choose a reason for hiding this comment

Uh oh!

sam-github Dec 5, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell Dec 7, 2016

Choose a reason for hiding this comment

Uh oh!

sam-github commented Dec 13, 2016

Uh oh!

kzurawel commented Dec 13, 2016

Uh oh!

sam-github commented Dec 13, 2016

Uh oh!

Trott commented Dec 21, 2016

Uh oh!

kzurawel commented Dec 21, 2016

Uh oh!

Trott commented Dec 22, 2016

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

sam-github Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

sam-github Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented Dec 23, 2016 • edited by gibfahn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

doc: document process.versions.modules#9901
kzurawel wants to merge 2 commits into
nodejs:masterfrom
kzurawel:process_versions_modules

Trott commented Dec 23, 2016 •
edited by gibfahn
Loading