docs!: add engines.node to package.json

[MichaelDeBoey]

Description

This makes it clear what the minimum supported Node version is

Linked Issues

N/A

Additional context

N/A

[benmccann]

Is there anything in this project actually requiring Node 18? We probably need Node 16 for the node: prefix in the imports

[MichaelDeBoey]

@benmccann Not that I know of

I just took the minimum version that's ran in CI 🤷‍♂️

[benmccann]

I doubt the library needs it, but it looks like our CI only runs on Node 18 because of pnpm. Personally, I'm not sure if I'd proceed with this PR as it would be a breaking change and unnecessarily prevents people from using this on Node 16. But if the other maintainers want to do that in a version 0.3 I won't object

[MichaelDeBoey]

@benmccann This PR is not a breaking change because we just never officially supported other Node versions.

A PR that would lower it to v16 would also not be a breaking change as that would just allow more people to install this package.

[antfu]

I don't feel that for this particular package we need to enforce the engine version - it doesn't have any specific requirements - we could document the officially supported versions but I don't feel the runtime error would be necessary if it still works anyway

[MichaelDeBoey]

@antfu for me personally (and I know a lot of people that do it the same way) I always start by looking at the package.json before looking at docs.

Having it in package.json can also automate things like having the warning or packages like check-engines

[benmccann]

you could setup the CI to conditionally run a different version of pnpm based on the node version so that we can test against node 16. Then we wouldn't have to artificially raise the supported versions

[antfu]

Having it in package.json can also automate things like having the warning or packages

I think that's the part that makes me hesitate about. I agree Ben's point that "we wouldn't have to make it hard artificially". To me, this is very simple package and ideally, it doesn't really require a specific Node version. To document a version would be better for communicating the our expected scope of responsibility (which is fair as we didn't have a test for v16 or so). But I just think people should be able to use any version they want (as long as it works) and take their own risk to do so.

[MichaelDeBoey]

@antfu Respectfully, I don't agree.

I think making it clear that people can only rely safely on certain versions is the better way.
People tend to not look (often) at docs (especially for these kind of things), so having it in engines.node makes it more obvious.

If people don't want to get the npm warning, they can also still disable it

[antfu]

Alright, let's have it then, but I consider this breaking so it should be a minor bump.

[benmccann]

Another option might be to switch out pnpm for npm. I love pnpm's speed, but we don't really need it here since it's not a monorepo. Then we could easily test against Node 16 on the CI without having to jump through hoops

[antfu]

On the other hand as Node 16 is already EOL I don't see it's valuable for the effort to migrate our setup back to "test" that.

I think we either:

1. Don't do anything (not having this PR), that implicitly supports Node 16 or even below
2. Or we can have this PR as a breaking change and drop Node 16 because we are not testing it

[benmccann]

Personally I would close this PR as I think it does more harm than good. If I'm on a recent supported version of Node I assume every package works and don't bother checking. It's only if I'm on an older version of Node that I start checking whether a package is supported before using it and in that case I would rather have no engines field documenting it than one that falsely claims the code only works on Node 18

[MichaelDeBoey]

a recent supported version of Node

@benmccann Since Node 16 is EOL as of 2023-09-11, I personally don't consider Node 16 as a "recent supported version", so I would merge this PR as a breaking change @antfu

[benmccann]

I agree Node 16 is not a recent supported version. What I'm saying is that when I'm working on a project that only supports Node 18+ I don't bother checking the engines field because it's extremely rare that I've come across a library that has already dropped support for Node 18 and so in that case the engines field doesn't really provide value in my mind. To me it's more about saying where the cutoff is if you're drawing the line somewhere further back

[benmccann]

I also worry that this would cause us to release many more breaking changes in the future. Essentially every time a new version of Node comes out we have to bump the engines field and release a breaking change especially if we want to align it with the versions pnpm allows us to test against. That causes unnecessary churn in the ecosystem and prevents this library from being deduped by package managers

[MichaelDeBoey]

Essentially every time a new version of Node comes out we have to bump the engines field and release a breaking change

@benmccann Only if you don't support the oldest version anymore. If you're just adding Node 23/24 now, that wouldn't be a breaking change.

especially if we want to align it with the versions pnpm allows us to test against

Nobody is forcing us to upgrade pnpm right away, we can always wait until we want to stop supporting older Node versions, at which point we would release a major release (which is how it should be)