Specify minimum node version number

[benmccann]

It's currently specified in

kit/packages/create-svelte/template/package.json

Line 17 in 96cbda3

"node": ">= 12.17.0"

but I think that's the wrong place because it still seems like people are hitting this (#724 #732). I think we may need to specify it in Kit since that's where the CLI comes from

[Conduitry]

Have you looked into how different package managers handle this? Is the warning only printed when there's a dependency that has an unmet pkg.engines.node value, or should it also be getting printed when installing the dependencies of a package with an unmet pkg.engines.node, and the warning is just too hard to notice?

If create-svelte is apparently running for the folks in those issues you linked (since they were able to get to the step of trying to run the app), does it make sense to add an explicit manual runtime check to the create-svelte CLI?

[Conduitry]

Okay, it does look like, at least from my test just now in npm, your top-level pkg.engines.node doesn't matter - just that of any dependencies you're trying to install. So moving this pkg.engines.node does seem like the right choice. It is a very missable warning though, and I think there would be value in having create-svelte outright refuse to run if saw you were on an unsupported version.

[Conduitry]

I'm not sure how to check what npm init's behavior is, but npx at least doesn't seem to have any special handling. Rollup requires Node 10 now, so in Node 8 I just tried running npx rollup and I got a syntax error rather than a helpful warning from npm. If npx doesn't first check this, it seems unlikely that npm init would, so putting pkg.engines.node on create-svelte probably wouldn't be too helpful.

[benmccann]

I wonder what if we add an .npmrc with engine-strict=true?

[Conduitry]

We could have one included in the template - and it looks like having that does prevent me from installing, say, latest Rollup on Node 8. But doing that bugs me for some reason. I don't know that I can adequately explain it.

[benmccann]

The only reason it'd bug me is because it seems like it should be the default behaviour and we shouldn't have to turn it on, but given that it's the way npm works I'm not sure if there's any better solution

[dummdidumm]

It may be better dx to create a commonjs version of the cli (if that's not already the case) and check process.version there and print a good error message there.

[benmccann]

I don't think the message that npm gives is hard to understand. The only issue right now is that it's not triggering at all

[dummdidumm]

Even with the .npmrc file? If so, process.version is our best bet I guess.

[Conduitry]

No, with the .npmrc npm bails when installing the dependency. Without it, it just prints a missable warning.

(This is all with the engines field on the child dependency, where it belongs, not in your app's package.json. I don't think that part of this PR is controversial.)

[Rich-Harris]

is this good to go? if so, 👍 — i couldn't tell if there were further changes planned

[Conduitry]

I'm happy with the engines change. I'll live with the .npmrc change. I have opinions about the lack of capitalization in and the vagueness of the changeset message.

[benmccann]

I thought you were saying that npm init wouldn't even give a warning without the .npmrc, which is why I added it. Not sure if I understood correctly

I've updated the changeset message