Possible breaking change in node-addon-api 5.1.0

[JckXia]

Hello folks, it looks like we had some compilation issues related to the introduction of the napi_callback_info implicit conversion operator. Issue link. Not too sure why this wasn't caught by our windows CI.

[JckXia]

(Just going to move our discussion over here for better visibility)

@mhdawson I think there might be a few options we can explore.

1. Remove the implicit conversion operator and declare the removal change a SemVer-major. Then, instead of using an implicit conversion operator, we can create an accessor function like GetCbInfo. This way people that need the raw napi_callback_info (hopefully not many since it's a fairly new change) still have a means of accessing that pointer. We also get to keep the existing API for [] overload, since a LOT of people (including us) are passing literals into info. The downside is that it seems to break conventions as we have been using implicit conversion operators to translate C++ NAPI objects to C node-API objects. But looking at std::string, there is an explicit getter c_str() function to acquire the C string equivalent makes me think if we should reconsider our approach.

2. Change the parameter type of the CallbackInfo::operator[](size_t index) to be int instead. I believe this way we should be able to avoid the ambiguity in question. But I don't know if this will be a breaking change for anyone because I don't understand all the nuances between size_t and int other than that size_t can potentially represent a greater range of values.

3. Remove the implicit conversion operator completely, but then people who might need the raw napi_callback_info is now out of luck if they had already written code around being able to access this pointer.

On a side note, I couldn't reproduce this error locally using MSVC 2019 C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Current\Bin\MSBuild.exe. Don't really know why that is. Going to take a deeper dive tomorrow.

[KevinEady]

Could we make the conversion function an explicit conversion only?

explicit operator napi_callback_info() const;

Then the user would need to use a static_cast<napi_callback_info>( cbInfo ) where cbInfo is an Napi::CallbackInfo object. A little more wordy but might work?

[KevinEady]

I forked the sharp repo at the commit where the issue was happening and pointed to my node-addon-api branch with the explicit cast. The compilation now succeeds (some job failed for what looks to be unrelated issues), eg. this successful job https://github.com/KevinEady/sharp/actions/runs/3944686124/jobs/6751171701 was failing before https://github.com/lovell/sharp/actions/runs/3924810417/jobs/6709314064 . Maybe this would be the best approach

[JckXia]

Some interesting find regarding the build issue not showing up in our current windows CI. I tweaked the setup-node@v3 action by setting its architecture to x86 in this branch node-addon-api windows CI with x86 all a sudden this compilations error c2666 is showing up. CI run results.

Did some digging into setup-node and it appears by default it is targeting x64 arch, and that is what our windows CI is testing against. What's interesting is that sharp didn't have an issue building node with x64 as the target arch either. Not sure if this is a setup-node problem, MSVC settings or has to do with node itself, seeing as the other commentator who experienced a similar issue didn't set the architecture option to x86 but opted to use the npm --arch=ia32 cli flag instead. node-sword-interface CI run

[mhdawson]

Then the user would need to use a static_cast<napi_callback_info>( cbInfo ) where cbInfo is an Napi::CallbackInfo object. A little more wordy but might work?

This seems reasonable to me.

[mhdawson]

Any concerns with this suggestion?

Then the user would need to use a static_cast<napi_callback_info>( cbInfo ) where cbInfo is an Napi::CallbackInfo object. A little more wordy but might work?

If we are going to do this we might want to do it soon and then publish a 6.x

[kevinGodell]

I noticed breaking changes from 5.0.0 to 5.1.0 on my pi. It only occurred on the 32bit versions of pi os (tried buster and bullseye) and both failed to compile.

just installed your latest commits directly from github npm install nodejs/node-addon-api and the 32bit pi is happy now and compiles ok. Thanks. Can't wait for the update to be published.

[NickNaso]

The new release v6.0.0 has been published I think that now it's possible to close thi issue if not please do not esitate to reopen.