Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4.9.0 needs minimum nodeJS 14 out of the blue without a word even in release notes #2970

Closed
KevinGossentCap opened this issue Nov 25, 2020 · 14 comments

Comments

@KevinGossentCap
Copy link

Before
Using previous version in deep dependency with nodeJS 12 was working

Now since 4.9.0
seems a mandatory minimum nodeJS version 14 have been forced into module.
It breaks quite some CI chains not ready yet for nodeJS14
And it's not even listed in release notes even though it might be a compatibility breaking thing

@cmoesel
Copy link

cmoesel commented Nov 25, 2020

Yes, this broke our CI overnight and also broke many of our user installations (since we have our npm dependency defined as ^4.8.0, which allowed for the automatic updates to 4.9.0). Does this module not follow semantic versioning or was this an oversight?

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Nov 26, 2020 via email

@ericvergnaud
Copy link
Contributor

Yes, this broke our CI overnight and also broke many of our user installations (since we have our npm dependency defined as ^4.8.0, which allowed for the automatic updates to 4.9.0). Does this module not follow semantic versioning or was this an oversight?

There is no commitment from Antlr to stick to the npm recommendation.
Rather Antlr favors versioning consistency across targets over semantic versioning within each target.
As of writing, we have 10 runtimes consistent with each other.

@duc-gp
Copy link

duc-gp commented Nov 26, 2020

i'm not sure what this now means. so we need to use node14 for our CI now to get this update, right? Otherwise we just stay at 4.8 and dont update?

@KevinGossentCap
Copy link
Author

KevinGossentCap commented Nov 26, 2020

@duc-talentwunder Yes that's interpretation. And also that regarding antlr4 we'd better stick to never allow auto update (or maybe max of patch with something like ~4.7.0 that would accept 4.7.x but not take any higher 4.y.x)
In your case you'd have to put "4.8.0" or max "~4.8.0"

@ericvergnaud
Copy link
Contributor

Yes if you want to upgrade you need to use node14 and update your code to use module semantics.

@youngpayters
Copy link

Well that is all well in good but you have introduced a breaking change that allow auto-updating to the latest version.
And your recourse is "tough, why you using relying on auto-updating" is poor to say the least.

This project has dependencies with ^ and I'm sure you wouldn't be happy if babel had a breaking change and you were auto upgraded to.

I don't use this project directly but this has a dependency on a project I do use, amazon-dax-client, which has caused this upgrade to break for us. We use this client in a lambda and AWS only support up to node 12. So changing the engine to 14 has just broken all lambdas that may use the dax client or the antlr library directly.

FYI, we are testing out using https://www.npmjs.com/package/npm-force-resolutions to ensure that any dependencies use 4.8.0.

For anyone interested, put this in your package.json after installing npm-force-resolutions

"resolutions": {
  "antlr4": "4.8.0"
},
"scripts": {
  "preinstall": "npx npm-force-resolutions",
  ...
}

@ericvergnaud
Copy link
Contributor

Well not sure who to blame, me or package providers who make unverified assumptions about antlr4 versioning? And why is AWS not supporting latest node?

@jantimon
Copy link

jantimon commented Nov 26, 2020

This is the standard and best practice for all npm packages..
Most npm users assume that a public package follows the official guidelines

4.9.0 also broke our pipeline because of another breaking change: #2968

@pbromley
Copy link

We're experience the same thing @youngpayters. We've had to declare the exact version as a direct dependency in our project which is not ideal since we don't use it directly.

"dependencies" : { "antlr4": "4.8.0" }

@cmoesel
Copy link

cmoesel commented Nov 26, 2020

@ericvergnaud -- I understand the advantage of having a consistent version across all targets. Given that, could you perhaps document very clearly (in bold letters) that antlr4 does not follow semantic versioning and that breaking changes may be introduced in minor releases? This might help future users avoid this issue so they will know to use ~ rather than ^.

I'd love to see that disclaimer on the antlr4 NPM page -- which, by the way, currently says that antlr4 runs on Node 3.10.9.

This is especially important because whenever you use npm install --save-dev it automatically applies the ^ in package.json -- so users must take an extra step to replace that with ~.

@ericvergnaud
Copy link
Contributor

@cmoesel good point, this will happen with the next patch

@fvictorio
Copy link

One problem with this is that previous versions of javascript-targeted ANTLR parsers had an annoying warning on node 14 (see #2951). So if you want to avoid that warning, you need to upgrade antlr to 4.9.0, which makes sense.

The problem is that if you do that, then you can only support node >=14. So if you want to support, say, node >=12, you need to maintain two different versions (and each package that depends on your package needs to do the same).

Put another way: antlr 4.8 works fine with node up to 13, and antlr 4.9 works with node >=14, so there's no way to support all the LTS, maintained versions of node (10, 12, 14) with a single antlr dependency.

@ericvergnaud
Copy link
Contributor

@fvictorio #2951 root cause was there since the origin and did not affect the runtime behavior so it's totally safe to ignore the warning

the original runtime was written in 2014, before the class keyword even appeared in the JS spec, and at a time many packaging frameworks were competing with each other (which led me to originally not package the runtime at all)
since then the runtime only evolved for bug fixes and minor improvements

with the rapid evolution of javascript the ask to convert the runtime to modern javascript and make it a standard module has been consistent, and @carocad was brave enough to engage this work which I completed later

the 4.9 rewrite has been generally well received, see #2787 (comment)

it does come with limitations, due to node itself, which only supports import and export starting with version 14

there are many ways to workaround these limitations, such as forcing 4.8 usage, using babel to make the code es2015 compatible, or others, but with so many toolchains out there we believe it's best to leave that to users themselves

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants