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.

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

meta: Indicate compatible Node.js versions #152

Closed
wants to merge 1 commit into from

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented Aug 28, 2024

Indicate compatible and supported Node.js versions in package manifest via engines.node.

@legobeat legobeat changed the title meta node version meta: Indicate compatible Node.js versions Aug 28, 2024
@legobeat legobeat marked this pull request as ready for review August 28, 2024 05:38
@paulmillr
Copy link
Owner

I don't think that's correct. We're likely supporting v16, v14, v12 etc.

Noble-hashes minimum is v16, but curves are usable without hashes. Esp if user brings their own hashes.

@legobeat
Copy link
Contributor Author

I don't think that's correct. We're likely supporting v16, v14, v12 etc.

Noble-hashes minimum is v16, but curves are usable without hashes. Esp if user brings their own hashes.

Oh!

Do you agree that it would make sense to rewrite the tests to be runnable under these Node.js versions, then, so compatibility can be verified? Currently they contain syntax that make the test suite require 18+.

@legobeat
Copy link
Contributor Author

legobeat commented Aug 28, 2024

@legobeat legobeat marked this pull request as draft August 28, 2024 23:30
@paulmillr
Copy link
Owner

I don’t know how to test v14, because nodejs 14 on arm macs is unavailable.

We are falling back to built in nodejs crypto instead of less supported webcrypto AFAIR. Also there is no hard dependency on webcrypto in most places. Lib can be used without randomness.

Currently they contain syntax that make the test suite require 18+.

such as?

@legobeat
Copy link
Contributor Author

legobeat commented Aug 29, 2024

We are falling back to built in nodejs crypto instead of less supported webcrypto AFAIR.

For consideration:

@legobeat
Copy link
Contributor Author

legobeat commented Aug 29, 2024

Currently they contain syntax that make the test suite require 18+.

such as?

The new syntax for loaders but that can be addressed!

I don’t know how to test v14, because nodejs 14 on arm macs is unavailable.

It looks like it should be doable now?

nodejs/node#40127
https://github.com/nodejs/node/blob/main/BUILDING.md#platform-list

I guess in any case if that is a concern it might make sense to test macOS in CI also?

@legobeat
Copy link
Contributor Author

legobeat commented Sep 1, 2024

@paulmillr Updated to indicate support also for v14, v16 (b69a8bb).

Indicating only support for at least latest patch releases for ^14.21.3 (2023-02-16) and ^16.20.2 (2023-08-08).

When attempting to run on Node.js v12, I get syntax error on the unsupported ?. syntax here. Looks like this syntax has been present since pre-1.0.0.

Is it worth it to fix support for v12? And in any case could be worth indicating what's currently expected to work? :)

@legobeat legobeat marked this pull request as ready for review September 1, 2024 21:39
@paulmillr
Copy link
Owner

  1. Why is it useful to specify supported versions?
  2. Why is it useful to specify it in curves in addition to hashes, which already specify it?

@paulmillr
Copy link
Owner

added manually

@paulmillr paulmillr closed this Sep 2, 2024
@legobeat legobeat deleted the meta-node-version branch September 8, 2024 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants