search

gajus / eslint-plugin-jsdoc

JSDoc specific linting rules for ESLint.
Other
1.09k stars 157 forks source link

Overly strict node versions specified in engine #774

Closed macsj200 closed 3 years ago

macsj200 commented 3 years ago

I'm using node 14.2.0 and encountering the following error:

The engine "node" is incompatible with this module. Expected version "^12.20 || ^14.14.0 || ^16". Got "14.2.0"

I would be surprised if this package only works on 14.14.0. Is there a way we can specify a more permissive node version restriction?

Something like 14.x would probably do the trick.

brettz9 commented 3 years ago

At one point, comment-parser only made certain utilities available within submodules. The engines for comment-parser were bumped as a result of the form of its exports not being compatible with earlier Node and we followed suit here. Earlier Node had a problem resolving exports like this:

    "./parser/*": {
      "import": "./es6/parser/*.js",
      "require": "./lib/parser/*.cjs"
    },

However, comment-parser has since made available the utilities which we need in this project (and in @es-joy/jsdoccomment on which we depend) as part of its main export, and since we are already accessing the utilities from the main export, we could now revert to using the lower Node requirements.

However, so long as comment-parser is still using the higher engines threshold, it shouldn't be helpful (or technically accurate) for this plugin to do so. I'm not sure whether he'll want to revert since some utilities may still only be accessible through submodules.

foray1010 commented 3 years ago

@brettz9 comment-parser has reverted to >=12.0.0 in v1.2.4, think we are ok to revert now? https://github.com/syavorsky/comment-parser/blob/master/CHANGELOG.md#v124

gajus commented 3 years ago

:tada: This issue has been resolved in version 36.0.8 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

amed commented 2 years ago 
The engine "node" is incompatible with this module. Expected version "^12 || ^14 || ^16". Got "15.14.0"

What about 15.x?

sauloquirino commented 2 years ago

The engine "node" is incompatible with this module. Expected version "^12 || ^14 || ^16". Got "17.0.0"

elektronik2k5 commented 2 years ago

So I encountered this artificial roadblock as well, with node 17. I don't understand why on Earth would a linter configuration care about the runtime. 🤷🏻‍♂️ What exact problem does this solve?

Anyways, I made a fork just to overcome this issue until it is resolved. Here's the required change to use it:

-    "eslint-plugin-jsdoc": "^36.1.1",
+    "eslint-plugin-jsdoc": "elektronik2k5/eslint-plugin-jsdoc#patch-1",

Cheers! 🍻

brettz9 commented 2 years ago

@amed : Node 15 is no longer in LTS .

@elektronik2k5 : The problem it solves is that it communicates that the linter has been tested on the given version, and it prompts us to ensure our CI is checking the given engine.

Planning to allow Node 17 as a part of #792 .

brettz9 commented 2 years ago

Also note that the engines restriction should only be enforced if the user has opted into the engine-strict flag; see https://docs.npmjs.com/cli/v7/configuring-npm/package-json

elektronik2k5 commented 2 years ago

@brettz9 that's the thing: it doesn't only communicate, but also enforces it, unnecessarily.

I use yarn for many reasons, one of them is better defaults. Specifically, unlike npm, it enforces engines by default - which in my opinion is a good thing. Likewise, when I'm forced to use npm, I always set the non default engine-strict=true flag on the project level via .npmrc. In such a configuration, eslint-config-jsdoc fails to install in any "non supported" version of node, unnecessarily.

In my opinion, there are two completely different concerns here:

  1. eslint-config-jsdoc should be tested in the versions of node which the project maintainers choose to support. That's possible in whichever CI vendor the project uses. This policy is meant for the maintainers of the project and has nothing to do with users.
  2. eslint-config-jsdoc should be installable and run in any compatible runtime. In this sense, JS aims to be a forward compatible language, where one of the language's guiding design principles is backwards compatibility ("don't break the web" and all that). Node of course is much more than just a JS runtime and has semver for good reasons, but I argue that for a linter configuration the chances of this package breaking as a result of a node upgrade are extremely low.

As such, the current use of the engines restriction is misused to enforce an overly strict policy for the wrong audience.

I suggest to follow the practice most node package maintainers have: a minimum version only. This way, users can install and use eslint-config-jsdoc on any technically compatible past, present or future version of node (starting from the earliest version which is capable of running the shipped JS).

In addition, in order to not deal with non supported versions, I suggest to declare (in the README and in the GitHub issue template) which versions of node the project supports and is tested against, instead of enforcing it in any way. This way, non tested versions are out of support scope, but if a user chooses to ignore the official policy then it can still install and work. If it doesn't, that's the user's problem and not the maintainers'.

But don't prevent people from using your package if they know what they're doing!

brettz9 commented 2 years ago

Appreciate your arguments, but I think your two items are in a sense connected for us, in that we become informed of the need to test new versions by our users if we haven't yet updated it ourselves. Rather than allowing users to encounter subtle bugs (or even potentially harmful ones) due to a breaking Node change, I believe it is better to enforce the changes, especially since we are actively maintaining the project and can update in relatively short order. If someone "knows what they're doing", they should be able to use their package manager to opt in to override the safety checks.

As much as some may say that the README is required reading, I tend to believe in more intrinsically safe and agile-friendly approaches. (And there are other projects, including linting projects, which do use engines in this manner as well.)

We've added support for Node 17 so in any case, things should work for now.

jdforsythe commented 1 year ago

@brettz9 how about Node 19?

brettz9 commented 1 year ago

@jdforsythe : The latest version should already be working for Node 19.

batpurev commented 1 year ago

is it compatible yet with version 18?

dspace@dspace:~/dspace-angular-dspace-7.4$ yarn install yarn install v1.22.19 [1/4] Resolving packages... [2/4] Fetching packages... error eslint-plugin-jsdoc@38.0.6: The engine "node" is incompatible with this module. Expected version "^12 || ^14 || ^16 || ^17". Got "18.12.1"

brettz9 commented 1 year ago

is it compatible yet with version 18?

dspace@dspace:~/dspace-angular-dspace-7.4$ yarn install yarn install v1.22.19 [1/4] Resolving packages... [2/4] Fetching packages... error eslint-plugin-jsdoc@38.0.6: The engine "node" is incompatible with this module. Expected version "^12 || ^14 || ^16 || ^17". Got "18.12.1"

Yes, the latest version, 39.6.4 is compatible with 18 and 19...

batpurev commented 1 year ago

Hi. thanks for the response. I have yarn.lock file in which we have following: what should be changed to make it use version 39.6.4.

thanks


eslint-plugin-jsdoc@^38.0.6:
  version "38.0.6"
  resolved "https://registry.yarnpkg.com/eslint-plugin-jsdoc/-/eslint-plugin-jsdoc-38.0.6.tgz#b26843bdc445202b9f0e3830bda39ec5aacbfa97"
  integrity sha512-Wvh5ERLUL8zt2yLZ8LLgi8RuF2UkjDvD+ri1/i7yMpbfreK2S29B9b5JC7iBIoFR7KDaEWCLnUPHTqgwcXX1Sg==
  dependencies:
    "@es-joy/jsdoccomment" "~0.22.1"
    comment-parser "1.3.1"
    debug "^4.3.4"
    escape-string-regexp "^4.0.0"
    esquery "^1.4.0"
    regextras "^0.8.0"
    semver "^7.3.5"
    spdx-expression-parse "^3.0.1
```"
batpurev commented 1 year ago

39.6.4 sorry, i am noob here. Tried changing it to.

eslint-plugin-jsdoc@^39.6.4:
version "39.6.4"

but no luck. same error:

error eslint-plugin-jsdoc@38.1.6: The engine "node" is incompatible with this module. Expected version "^12 || ^14 || ^16 || ^17". Got "18.12.1"
error Found incompatible module.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
batpurev commented 1 year ago

changed following in package.json

"eslint-plugin-jsdoc": "^39.6.4"

but

following warnings and errors.

warning " > @nicky-lenaers/ngx-scroll-to@9.0.0" has incorrect peer dependency "@angular/common@^8.0.0 || ^9.0.0".
warning " > @nicky-lenaers/ngx-scroll-to@9.0.0" has incorrect peer dependency "@angular/core@^8.0.0 || ^9.0.0".
warning " > @nicky-lenaers/ngx-scroll-to@9.0.0" has incorrect peer dependency "tslib@^1.10.0".
warning " > bootstrap@4.3.1" has unmet peer dependency "jquery@1.9.1 - 3".
warning " > bootstrap@4.3.1" has unmet peer dependency "popper.js@^1.14.7".
warning " > ngx-sortablejs@11.1.0" has incorrect peer dependency "@angular/common@^11.0.0".
warning " > ngx-sortablejs@11.1.0" has incorrect peer dependency "@angular/core@^11.0.0".
warning " > ngx-ui-switch@11.0.1" has incorrect peer dependency "@angular/core@>=5.0.0 <12.0.0".
warning " > ngx-ui-switch@11.0.1" has incorrect peer dependency "@angular/common@>=5.0.0 <12.0.0".
warning " > ngx-ui-switch@11.0.1" has incorrect peer dependency "@angular/forms@>=5.0.0 <12.0.0".
error An unexpected error occurred: "ENOSPC: no space left on device, copyfile '/home/dspace/.cache/yarn/v6/npm-typescript-4.6.3-eefeafa6afdd31d725584c67a0eaba80f6fc6c6c-integrity/node_modules/typescript/lib/tsserverlibrary.js' -> '/home/dspace/dspace-angular-dspace-7.4/node_modules/@swc-node/register/node_modules/typescript/lib/tsserverlibrary.js'".
info If you think this is a bug, please open a bug report with the information provided in "/home/dspace/dspace-angular-dspace-7.4/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
brettz9 commented 1 year ago

If there is something relevant we can do, please file a new issue pointing to a minimal test repo, but those warnings and error do not look relevant to our package.