search

verida / verida-js

The Verida SDK provides several SDKs to interact with the Verida Network
https://developers.verida.network
ISC License
1.31k stars 37 forks source link

update to node 18 and update lerna. Closes #413 #415

Closed nick-verida closed 5 months ago

aurelticot commented 6 months ago

It's not as simple as updating .nvmrc, please see details in https://github.com/verida/verida-js/issues/402

nick-verida commented 6 months ago

While all the thing in #402 are probably correct we've been running things under node 18 for a while (not node 20). Also for a library it seems to make sense to move to the lowest possible version so we don't lockout other projects that are on node 18.

We have to do something and this seems extremely safe.

aurelticot commented 6 months ago

While all the thing in #402 are probably correct we've been running things under node 18 for a while (not node 20). Also for a library it seems to make sense to move to the lowest possible version so we don't lockout other projects that are on node 18.

We have to do something and this seems extremely safe.

I disagree.

This PR only updates .nvmrc which is only used for dev, tests and build. The engine requirements is what's important and what constrain other projects.

The engine is currently defined with node >=14 but our tests are run with whatever is defined in .nvmrc so if it is node 18, there is no guarantee our libraries will work on node 14 to 17. So, no, it is not extremely safe!

It is reasonable to think no projects should run on node lower than 18 (actually even 20), so if we "have to do something", it is about upgrading the engine requirement. It's a breaking change and the purpose of #402, which highlight broader consideration, with changes across package.json, tsconfig.json and .nvmrc. That's my point: it's not as simple as only updating .nvmrc.

nick-verida commented 6 months ago

The engine is currently defined with node >=14 but our tests are run with whatever is defined in .nvmrc so if it is node 18, there is no guarantee our libraries will work on node 14 to 17. So, no, it is not extremely safe!

Yes this is a good point. But it's still better than now where the tests run in node 14 (which - again - is about to be out of maintenance)

I agree we should do all the things in #402 (although I think I'd still argue for node 18 instead of node 20).

I don't have time to do all of them now though, and by doing this change at least we'd test under the same node version we run things on!

nick-verida commented 6 months ago

(I was going to update the version in package.json too but clearly we don't want to do that since we still want to allow people to use earlier versions if they choose)

aurelticot commented 6 months ago

I agree we should do all the things in https://github.com/verida/verida-js/issues/402 (although I think I'd still argue for node 18 instead of node 20).

402 suggests node 20 in .nvmrc only if we run the tests on multiple node version, otherwise .nvmrc should be aligned with the engine requirement.

(I was going to update the version in package.json too but clearly we don't want to do that since we still want to allow people to use earlier versions if they choose)

402 suggests the engine requirement to be node >= 16, to take into account that 14 is obsolete but 16 is still maintained and we want to support projects still using it.

by doing this change at least we'd test under the same node version we run things on!

"... we run things on" but we are not the only one running things on. At this stage it's fair to say we are the main consumer of the SDK and we know who use it and can inform them there's a lack of test converage on earlier supposedly supported node version. But I personally don't want this to be a precedent, so by principle I will stay against this PR as it is. I'll leave @tahpot with the decision to merge it.

nick-verida commented 5 months ago

This now fixes the docs and updates lerna too