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.

Sign up for GitHub

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

Jump to bottom

src: describe what NODE_MODULE_VERSION is for #10414

Closed
wants to merge 1 commit into from
Closed

src: describe what NODE_MODULE_VERSION is for #10414

wants to merge 1 commit into from

Conversation

sam-github
Copy link
Contributor

Current comment described what to do with it when the ABI changes, but
implied that node would load modules with newere ABI numbers, which it
will not.

Checklist
  • commit message follows commit guidelines
Affected core subsystem(s)

src

Description of change

@sam-github
Copy link
Contributor Author

/to @addaleax cf. https://github.com/nodejs/node/pull/9901/files#r93670565

@addaleax addaleax added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 22, 2016
addaleax
addaleax approved these changes Dec 22, 2016
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM but there’s a commit message typo (newere)

@sam-github
src: describe what NODE_MODULE_VERSION is for
2f5b9a4 
Current comment described what to do with it when the ABI changes, but
implied that node would load modules with newer ABI numbers, which it
will not.
@sam-github sam-github force-pushed the complete-modules-version-comment branch from b99ec8d to 2f5b9a4 Compare December 22, 2016 18:45
@sam-github
Copy link
Contributor Author

Typo fixed, thanks.

@Trott Trott mentioned this pull request Dec 23, 2016
Closed
3 tasks
@Trott
Copy link
Member

Trott commented Dec 23, 2016

We're standardizing on ABI number instead of ABI version or some other formulation? I'm OK with whatever, but I want to make sure we pick one thing and stick with it everywhere. (I've changed an instance of ABI version to ABI number in #10419. There is an additional one in releases.md.)

Trott
Trott reviewed Dec 23, 2016
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Do we need to change the comment below that now repeats a fair bit of this new comment?

@sam-github
Copy link
Contributor Author

We're standardizing on ABI number instead of ABI version or some other formulation?

I didn't intend to imply that, I just called it a number because it is, I'll change it back to be "version" in #10149

I really regret that modules got added to process.versions, but that's water under the bridge.

thefourtheye
thefourtheye approved these changes Dec 26, 2016
View reviewed changes
Copy link
Contributor

@thefourtheye thefourtheye left a comment

Choose a reason for hiding this comment

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

Wouldn't ABI version be better, instead of ABI number?

jasnell pushed a commit that referenced this pull request Dec 27, 2016
@sam-github @jasnell
src: describe what NODE_MODULE_VERSION is for
1257546 
Current comment described what to do with it when the ABI changes, but
implied that Node.js would load modules with newer ABI numbers, which it
will not.

PR-URL: #10414
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@jasnell
Copy link
Member

jasnell commented Dec 27, 2016

Landed in 1257546

@jasnell jasnell closed this Dec 27, 2016
@evanlucas evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
@sam-github @evanlucas
src: describe what NODE_MODULE_VERSION is for
fac9c4d 
Current comment described what to do with it when the ABI changes, but
implied that Node.js would load modules with newer ABI numbers, which it
will not.

PR-URL: #10414
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
@sam-github @evanlucas
src: describe what NODE_MODULE_VERSION is for
11ed800 
Current comment described what to do with it when the ABI changes, but
implied that Node.js would load modules with newer ABI numbers, which it
will not.

PR-URL: #10414
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@sam-github @MylesBorins
src: describe what NODE_MODULE_VERSION is for
57f233f 
Current comment described what to do with it when the ABI changes, but
implied that Node.js would load modules with newer ABI numbers, which it
will not.

PR-URL: #10414
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@sam-github @MylesBorins
src: describe what NODE_MODULE_VERSION is for
968644a 
Current comment described what to do with it when the ABI changes, but
implied that Node.js would load modules with newer ABI numbers, which it
will not.

PR-URL: #10414
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@sam-github @MylesBorins
src: describe what NODE_MODULE_VERSION is for
78d85c6 
Current comment described what to do with it when the ABI changes, but
implied that Node.js would load modules with newer ABI numbers, which it
will not.

PR-URL: #10414
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@sam-github @MylesBorins
src: describe what NODE_MODULE_VERSION is for
7c3486b 
Current comment described what to do with it when the ABI changes, but
implied that Node.js would load modules with newer ABI numbers, which it
will not.

PR-URL: #10414
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@sam-github @MylesBorins
src: describe what NODE_MODULE_VERSION is for
175ffda 
Current comment described what to do with it when the ABI changes, but
implied that Node.js would load modules with newer ABI numbers, which it
will not.

PR-URL: #10414
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
This was referenced Jan 24, 2017
Merged
Merged
@sam-github sam-github deleted the complete-modules-version-comment branch January 25, 2017 21:10
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
@sam-github @MylesBorins
src: describe what NODE_MODULE_VERSION is for
7da0608 
Current comment described what to do with it when the ABI changes, but
implied that Node.js would load modules with newer ABI numbers, which it
will not.

PR-URL: #10414
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
@sam-github @MylesBorins
src: describe what NODE_MODULE_VERSION is for
be62037 
Current comment described what to do with it when the ABI changes, but
implied that Node.js would load modules with newer ABI numbers, which it
will not.

PR-URL: #10414
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Italo A. Casas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@gibfahn gibfahn gibfahn approved these changes

@italoacasas italoacasas italoacasas approved these changes

@santigimeno santigimeno santigimeno approved these changes

@addaleax addaleax addaleax approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@sam-github @Trott @jasnell @thefourtheye @addaleax @santigimeno @italoacasas @gibfahn @MylesBorins @nodejs-github-bot