Skip to content

Conversation

gabrielschulhof
Copy link
Contributor

Add macro NODE_MODULE_DECLARE_ABI to give addon authors the option of
providing a list of versions for various parts of the ABI against which
to check the addon at load time.

Re: nodejs/TSC#651

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 16, 2019
@gabrielschulhof gabrielschulhof added the addons Issues and PRs related to native addons. label Jan 16, 2019
Add macro `NODE_MODULE_DECLARE_ABI` to give addon authors the option of
providing a list of versions for various parts of the ABI against which
to check the addon at load time.

Re: nodejs/TSC#651
@gabrielschulhof
Copy link
Contributor Author

@mcollina
Copy link
Member

what's the semversiness of this change? I think it should be semver-major.

@addaleax
Copy link
Member

@mcollina I’m not quite sure about the use cases here/if it is worth the added complexity (because it most likely only changes later-occurring errors into earlier ones), but it seems semver-minor as it is an opt-in feature for addons?

@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 19, 2019
@refack
Copy link
Contributor

refack commented Jan 19, 2019

but it seems semver-minor as it is an opt-in feature for addons?

The addition of the macro seems semver-minor to me as well. But is it possible that the refactoring of node.h has semver-major impact?

@gabrielschulhof
Copy link
Contributor Author

@refack #include <node.h> after this change will define exactly the same things as now, plus the NODE_MODULE_ABI_* macros and the NODE_MODULE_DECLARE_ABI macro. Unless copying of the node.h header into your own project is part of our semver covenant, this is semver-minor.

@gabrielschulhof
Copy link
Contributor Author

I'm thinking that, for NODE_ABI_ENGINE_VERSION we may want to structure the number as a bit field to accommodate both engine type (V8/Chakra) and engine version. Or should we split it into two variables?

@refack
Copy link
Contributor

refack commented Jan 22, 2019

Unless copying of the node.h header into your own project is part of our semver covenant, this is semver-minor.

I agree. Considering that, does this change have a benefit before the next major version bump? If not maybe it's worth waiting just to be extra cautious? If it does, consider my comments non-blocking.

@gabrielschulhof
Copy link
Contributor Author

@refack so far in node_api.h we've pretty much been copying large-ish parts of node.h because node.h is C++ and node_api.h is C:

If we don't start factoring the C++-agnostic things out of node.h the overlap between the macros in node.h and node_api.h will get even larger. For example, the NODE_MODULE_ABI_*_VERSION have to be in a C++-agnostic header, otherwise they'd need to be copied into node_api.h with NAPI_MODULE_ABI_*_VERSION.

Having said this, we might simply wait until the next major version to land this feature. I mean, it'd make sense that the next major version of Node.js now has the new, cool Addon ABI Declarations™ feature.

@fhinkel
Copy link
Member

fhinkel commented Oct 28, 2019

I'm cleaning out a few old PRs 🧹. I'm closing this due to inactivity. Please re-open if needed!

@fhinkel fhinkel closed this Oct 28, 2019
@gabrielschulhof gabrielschulhof deleted the abi-version branch January 28, 2021 00:14
@gabrielschulhof gabrielschulhof restored the abi-version branch January 28, 2021 05:40
@gabrielschulhof gabrielschulhof deleted the abi-version branch February 3, 2021 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

addons Issues and PRs related to native addons. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants