Skip to content

Conversation

gabrielschulhof
Copy link
Contributor

Re: nodejs/abi-stable-node#332 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Oct 2, 2018
@vsemozhetbyt vsemozhetbyt added the node-api Issues and PRs related to the Node-API. label Oct 2, 2018
Copy link
Member

Choose a reason for hiding this comment

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

I might start with "An Application Binary Interface (ABI) is a way for programs to call other functions and use data structures from other compiled programs (binaries)" or something like that?

Copy link
Member

@benjamingr benjamingr Oct 3, 2018

Choose a reason for hiding this comment

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

ABI provider ('the library being required') and ABI user ('the consuming code') in the document maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't say "the library being required", because in the case of a native addon, it's actually the executable program that is being required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also wouldn't say "consuming" either, because that's too abstract. I can change it to "provider of the ABI" and "user of the ABI" to make it more down-to-Earth.

Copy link
Member

Choose a reason for hiding this comment

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

In practice, this means that Node.js commits that a native addon compiled against...?

Copy link
Member

Choose a reason for hiding this comment

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

multiple major Node.js versions ?

Copy link
Member

Choose a reason for hiding this comment

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

Just to be painfully obvious: ...and remains backwards compatible.?

Copy link
Member

Choose a reason for hiding this comment

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

I would link to the N-API and native-addon docs here perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean https://nodejs.org/docs/latest/api/addons.html and https://nodejs.org/docs/latest/api/n-api.html? I don't quite understand why I would link to the docs here?

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

I'm very +1 on adding this guide overall and it contains good content - left some nits -I think the start could be more new-user friendly in terms of motivating ABI.

@gabrielschulhof
Copy link
Contributor Author

@benjamingr I addressed your review comments, and I have questions about some of them.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

Landed in 65366ad.

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. node-api Issues and PRs related to the Node-API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants