jfmengels / node-elm-review

CLI for elm-review
https://package.elm-lang.org/packages/jfmengels/elm-review/latest/
BSD 3-Clause "New" or "Revised" License
48 stars 25 forks source link

Update Node engine version in package.json to `>=16.14` #133

Closed hingew closed 5 months ago

hingew commented 1 year ago

Description

The minimal node version should be 16.14.0. In a ci pipline with a lower version we got the following error:

$ npm ci --cache .npm --prefer-offline
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'lru-cache@10.0.0',
npm WARN EBADENGINE   required: { node: '14 || >=16.14' },
npm WARN EBADENGINE   current: { node: 'v16.13.1', npm: '8.1.2' }
npm WARN EBADENGINE }

Dependency tree for lru-cache

-> npm ls lru-cache
├─┬ elm-review@2.10.2
│ └─┬ rimraf@5.0.1
│   └─┬ glob@10.3.1
│     └─┬ path-scurry@1.10.0
│       └── lru-cache@10.0.0

According the docs from lru-cache (https://github.com/isaacs/node-lru-cache#breaking-changes-in-version-8) the engine version should be 16.14.0 or higher.

Tnks for elm-review :heart:.

lishaduck commented 5 months ago

https://github.com/jfmengels/node-elm-review/blob/e1958183e2e3355581296c1f865f7ef39bfa340c/package-lock.json#L10467 https://github.com/isaacs/node-lru-cache/blob/v9.0.0/package.json#L80-L82

On my Windows machine (my main Elm machine), I only have 16.12, so I've just had to ignore it for (two?) years. Luckily, it works, but npm complains mightily. I never thought to check if it what caused it, otherwise I would have noted it sooner :)

Originally posted by @lishaduck in https://github.com/jfmengels/node-elm-review/issues/147#issuecomment-2169064026

lishaduck commented 5 months ago

Sorry for all the noise, this issue can be marked as resolved.

Old issue

I did some more investigation: - `lru-cache` v10 re-adds support for Node 14, but, as @hingew noted, this `path-scurry` requires v9. - Latest `path-scurry` uses `lru-cache` v10, and latest `glob` uses latest `path-scurry`. So that all seems fine, what's pulling in glob? - It's `rimraf`, the latest of which supports `engines` `>=14.18`, so that too seems fine, and `elm-review` uses `^5.0.0 , so there shouldn't be an issue. It looks like this got fixed sometime in the last year, so maybe we should just refresh our lockfiles and pray it gets fixed. It seems fine, so it's either stale or a bug in npm. EDIT: I regenerated my lockfile, and while I still get this error, it's due to `node-elm-compiler` (through `elm-optimize-level-2`), `elm-doc-preview`, and `elm-pages@2`. Hurrah! elm-review for Node 14 (well, 16, but same difference)! I might suggest regenerating your lockfile as well. It's quite out of date.

jfmengels commented 5 months ago

Sorry for the delay in replying to this issue. Yes, older Node.js support broke underneath my feet, even more with the last release (mostly because of glob) if I'm not mistaken. I think in the future we should add a tool in the test runner to verify that we didn't break support for a Node.js version we ought to support (such as https://www.npmjs.com/package/ls-engines)

@lishaduck I'm sorry, I'm not sure what you mean by "this issue can be marked as resolved.", do you mean this issue #133? Can support for v10/12 been restored somehow? :thinking:

hingew commented 5 months ago

@jfmengels i can take a look at this.

jfmengels commented 5 months ago

Fixed by #157

lishaduck commented 5 months ago

I meant that latest lru-cache supports >=14. I'd forgotten that elm-review supports node 10 (despite having come here from that discussion 🤣)