syavorsky / comment-parser

Generic JSDoc-like comment parser.
MIT License
239 stars 24 forks source link

Why node 15 is not supported? #141

Closed m-bock closed 3 years ago

m-bock commented 3 years ago

https://github.com/syavorsky/comment-parser/blob/master/package.json#L49

This breaks CI on a dependent library.

syavorsky commented 3 years ago

Good point, I think it should only define the compatibility bottom line. Let's check with commit author. I am sorry for introducing it without thorough consideration

@brettz9 ping

brettz9 commented 3 years ago

Node 15, as an odd-numbered version, is technically not to be used on Production, so most projects I have seen don't check these versions in CI and projects also often avoid adding support for them when they use precise ranges in engines.

Still, if desired, I can experiment with the versions to see in which version proper nested exports were added, so you can resume with Node 15 support and thus all still-current compatible versions. Just know that this version will only be supported for a couple more months.

syavorsky commented 3 years ago

it sounds like comment-parser suggests what version dependant code should be using. I am going to revert it to "node": ">= 12.0.0"

brettz9 commented 3 years ago

That will be fine for people requiring the main comment-parser, but it will not be accurate for people using subpaths and an early Node version in the 12/14/16 lines.

jituanlin commented 3 years ago

Node 15, as an odd-numbered version, is technically not to be used on Production, so most projects I have seen don't check these versions in CI and projects also often avoid adding support for them when they use precise ranges in engines.

Still, if desired, I can experiment with the versions to see in which version proper nested exports were added, so you can resume with Node 15 support and thus all still-current compatible versions. Just know that this version will only be supported for a couple more months.

Don't teach users what they should do.

Which node version should be used must be decided by the user.

jituanlin commented 3 years ago

it sounds like comment-parser suggests what version dependant code should be using. I am going to revert it to "node": ">= 12.0.0"

Yes! It 's the right way!

brettz9 commented 3 years ago

Node 15, as an odd-numbered version, is technically not to be used on Production, so most projects I have seen don't check these versions in CI and projects also often avoid adding support for them when they use precise ranges in engines. Still, if desired, I can experiment with the versions to see in which version proper nested exports were added, so you can resume with Node 15 support and thus all still-current compatible versions. Just know that this version will only be supported for a couple more months.

Don't teach users what they should do.

Which node version should be used must be decided by the user.

If that were the case, then all versions of Node should be allowed. Instead, engines exists to flag only those versions which are supported. Not all versions of Node 15 may work with subpaths, so they really ought to be tested. On the other hand, it doesn't matter too much with Node 15 as a development version that won't be supported much longer by Node, so ^15.0.0 is probably fine for the Node 15 portion of the range.

syavorsky commented 3 years ago

@brettz9 can you clarify what exactly in comment-parser is compatible in 12 and 14 but not in 15. If there are such things I would rather let dependant projects (the actual runnable code) decide and set the engine

UPD: I should have rather said claimed to work in LTS versions but is broken in the next non-stable ones

compatible in 12 and 14 but not in 15

brettz9 commented 3 years ago

I have now confirmed that even 15.0.0 is indeed fine for the 15 line.

The 12 and 14 portion of the version range ^12.20 || ^14.14.0 || ^15 || ^16 (offered in PR #143) was set higher than ^12 || ^14 because code such as:

import tag from 'comment-parser/parser/tokenizers/tag';
const tag = require('comment-parser/parser/tokenizers/tag').default;

...would give the following error in those earlier versions:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './parser/tokenizers/tag' is not defined by "exports" in /Users/brett/comment-parser/package.json

require('comment-parser') and import {parser} from 'comment-parser' work fine even in the early 12/14 versions, however, so you could technically allow ^12 || ^14 and just mention the limitation for users if you wished. I just thought that users should get an early failure (or be notified in some way) that the package might not work properly for them and flag at which version it was completely safe for all purposes. But version 15 is fine for all versions. (I just wasn't sure whether it too had been more recently patched like 12 and 14, but indeed 15 was fine from the beginning.)

There is also the concern with the kind of approach taken in #142 that it is allowing Node versions 17+ without any confirmation that it will work. Note that engines only triggers a failure by those who opt into it, and those who opt into it, presumably want to be notified early of failures and people looking at package.json may falsely assume it should work when it doesn't. But again, as mentioned, it can partially work in those earlier versions, so if you want to revert the prohibition of earlier 12/14, I can adjust the PR.

syavorsky commented 3 years ago

comment-parser@1.2.4 is out with engine: ^12.0.0

this might not be the decision making everyone happy but it seems like a reasonable bottom line for compatibility manifest. I think dependant projects should be deciding on stricter engine constraints

The whole ESM/CJS/whatever-evertuning-node-tooling story turned to be disastrous from commit 0. I want to minimize non-functional changes from now on, unless there is a clear bug in the common use pattern.