Update ci-win.yml

[mhdawson]

[mhdawson]

@vmoroz do you know if VS2019 should still run with Node.js 22.x? I'm wondering if we dropped it.

[vmoroz]

@vmoroz do you know if VS2019 should still run with Node.js 22.x? I'm wondering if we dropped it.

@mhdawson , I see that Nodejs CI uses both VS2019 and VS2022. I would suggest to keep it until the next VS2025 is out.

Though, if it creates issues, then let's drop it. It is quite old.

[mhdawson]

@vmoroz there seem to be compiler failures with Node.js 22 and VS2019. Is that something you could take a look at?

"D:\a\node-addon-api\node-addon-api\test\build\binding.vcxproj" (default target) (6) ->
(ClCompile target) -> 
  D:\a\node-addon-api\node-addon-api\napi-inl.h(157): error C2102: '&' requires l-value [D:\a\node-addon-api\node-addon-api\test\build\binding.vcxproj]

"D:\a\node-addon-api\node-addon-api\test\build\binding.sln" (default target) (1) ->
"D:\a\node-addon-api\node-addon-api\test\build\binding_custom_namespace.vcxproj.metaproj" (default target) (7) ->
"D:\a\node-addon-api\node-addon-api\test\build\binding_custom_namespace.vcxproj" (default target) (8) ->
  D:\a\node-addon-api\node-addon-api\napi-inl.h(157): error C2102: '&' requires l-value [D:\a\node-addon-api\node-addon-api\test\build\binding_custom_namespace.vcxproj]

"D:\a\node-addon-api\node-addon-api\test\build\binding.sln" (default target) (1) ->
"D:\a\node-addon-api\node-addon-api\test\build\binding_noexcept.vcxproj.metaproj" (default target) (9) ->
"D:\a\node-addon-api\node-addon-api\test\build\binding_noexcept.vcxproj" (default target) (10) ->
  D:\a\node-addon-api\node-addon-api\napi-inl.h(157): error C2102: '&' requires l-value [D:\a\node-addon-api\node-addon-api\test\build\binding_noexcept.vcxproj]

"D:\a\node-addon-api\node-addon-api\test\build\binding.sln" (default target) (1) ->
"D:\a\node-addon-api\node-addon-api\test\build\binding_noexcept_maybe.vcxproj.metaproj" (default target) (11) ->
"D:\a\node-addon-api\node-addon-api\test\build\binding_noexcept_maybe.vcxproj" (default target) (12) ->
  D:\a\node-addon-api\node-addon-api\napi-inl.h(157): error C2102: '&' requires l-value [D:\a\node-addon-api\node-addon-api\test\build\binding_noexcept_maybe.vcxproj]

[vmoroz]

@mhdawson , I will fix them. Is it OK if I do it directly in this PR?

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 64.71%. Comparing base (62e8558) to head (aa4de9f). Report is 4 commits behind head on main.

Files	Patch %	Lines
napi-inl.h	0.00%	0 Missing and 1 partial :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1503 +/- ## ======================================= Coverage 64.71% 64.71% ======================================= Files 3 3 Lines 1981 1981 Branches 687 687 ======================================= Hits 1282 1282 Misses 138 138 Partials 561 561 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[legendecas]

do you know if VS2019 should still run with Node.js 22.x? I'm wondering if we dropped it.

Node.js v22 drops support building core with VS2019: see https://github.com/nodejs/node/blob/v22.x-staging/BUILDING.md#prerequisites.

[vmoroz]

do you know if VS2019 should still run with Node.js 22.x? I'm wondering if we dropped it.

Node.js v22 drops support building core with VS2019: see https://github.com/nodejs/node/blob/v22.x-staging/BUILDING.md#prerequisites.

Hmm, but we still run VS2019 in CI: https://ci.nodejs.org/job/node-compile-windows/56159/

Dropping VS2019 for node-addon-api sounds good to me.

I would say a much more important question is to decide on which C++ versions we support. Node.js 22 uses C++20, and we saw the new build breaks. Since Node-API allows building modules that work across multiple Node.js versions, we should also allow building C++ code that is not necessary matches Node.js C++ version.

I would say that it is reasonable to say that we support C++17, C++20, and latest C++. Supporting latest C++ may cause build breaks when GitHub VM images are updated, but we can align our code to the latest C++ standards and libraries sooner.

[StefanStojanovic]

I've been working on the Windows part of the CI for the past year or so. With that in mind, I'd like to share with you the state of CI regarding which machines are used for what, but before that, this document is probably a good reference for this discussion.

Compilation: yes we have both VS2019 and VS2022 for compiling Node.js. VS2019 is kept because of the LTS branches, so it is only used for v18 and v20 daily builds and PRs to backport something to those branches. VS2022 is used for compiling Node.js v21+, although v21 supported VS2019 compilation as well. IMHO there is no need to support VS2019 on any new version as even the part for finding a suitable VS2019 installation is removed.

Native add-ons: They are a different thing, and with them, we accept a broader range of VS versions, the latest change there is that VS2017 was dropped from Node.js v22. This is also the reason we have VS versions as far as VS2015 in the CI as we are running the native test suites with Node.js binaries compiled with either VS2019 (v18 and v20) or v2022 (v21+) and compiling native add-ons with VS2015-VS2022.

Hopefully, I was able to clear up some confusion and if there is something else you'd like to know about it, feel free to ask.

[NickNaso]

From the native add-on developer perspective it's a good thing have the support for VS2019.

[StefanStojanovic]

From the native add-on developer perspective it's a good thing have the support for VS2019.

Yes, and it is supported for native add-ons. We've just dropped VS2017, so VS2019 should be safe for now.

[mhdawson]

Ok, so I think current version with @vmoroz fixes does not drop any support but makes sure we can work across the board. Landing.