-
Notifications
You must be signed in to change notification settings - Fork 2k
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
What is the Node.js version requirement for 4.x? #1668
Comments
@ErisDS hey👋 I'm the maintainer of yargs. I saw the recent handlebars update that migrates from optimist to yargs (which is exciting) ... but, yargs@15.x.x requires Node 6, so might be breaking for some handlebars users (based on your engines field).
What are the version support goals for handlebars, I could work with you to target a slightly older version of yargs (but can't support all the way back to 0.4). I might just be tempted to call this a major change for people. |
There are actually tests that should have failed in this case, but even though I can see the error messages hidden in the Travis log, the build itself did not fail. The test tries to use the handlebars CLI with node versions starting at 0.10. @ErisDS I didn't see this coming. Shall we revert? |
@nknapp @ErisDS I think it could be fairly disruptive to folks if you don't revert, what if you reverted, and made it a You could install |
@dougwilson thanks for bringing this to our attention ❤️ Could you explain where and why you're running against Node v6 just so we have a better understanding? Me and @nknapp have had a chat and think that it's time for Handlebars' extensive Node compatibility to end as there aren't enough maintainers and quite frankly, I doubt there's any need for support < v6. What I'm going to do today is update the compatibility to be minimum Node.js v6 for now. I'll also downgrade yargs to v14 and merge @nknapp's changes to fix the tests. This should give us a working version that supports a sensible but very wide range of Node versions and doesn't have the minimist issue. We'll support v6 & v8 for a short while longer and determine a strategy and timeline for dropping those internally that we are comfortable with, based on any feedback from the community. @bcoe there's no expectation on our part that you would do any extra work to continue to support us on yargs v14. We really, really appreciate you showing up here today, joining the conversation and clarifying that yargs v14 would work and is secure. It has been immensely helpful ❤️ |
Also came here since we are experiencing issues on node v6.. our deps: karma-coverage@1.1.2 Thanks for the solution you are proposing @ErisDS, that should work for us too! |
The changes discussed here have been made and shipped in 4.7.5. Node.js support is now set to v6+ for 4.x, yargs is pinned to v14. Are you still using <v6 somewhere that this will actively impact? The idea of this change, rather than going straight to v10+ is that it should be enough to provide realistic compatibility within the constraints we have. I'm under instruction not to do major releases, so it's this or going back to having insecure dependencies unless someone else has a bright idea. |
@dougwilson I'm not unsympathetic to that at all. I'm genuinely sorry that this has caused you so much pain. Ultimately, all this new security reporting tooling is nice but it's definitely overbearing. Pretty much every piece of software I maintain now has multiple flags against it, largely for dev dependencies that affect no one. There's months of work needed to swap dependencies like was done here and I was incredibly grateful that in this case the work was done for us. So I think our pain is coming from the same place... With this module, we're stuck between a rock and a hard place. Do we favour the people whose builds are failing on Also, I understood from what you were saying that v6 support was going to be enough. Again, I'm sorry that it's not enough in your case. As I said I'm pretty out of ideas for what to do. |
@ErisDS thanks for your hard work here ❤️ I tend to agree with @dougwilson, that it's probably worth releasing this as a truly appreciate the hard work that you're doing for the community. |
Let me also clarify that I made todays changes based on the thread here, believing v6 was going to be enough for everyone who spoke up. So I feel a bit aggrieved that I'm now being called unsympathetic. It's difficult to advocate for a major bump to drop versions of node that are 2+ years past EOL. That's not to say I don't appreciate it's jarring and a lot of work for people, I do. It might be that downgrading yargs further or using a different dependency altogether would solve all the problems, but I only have capacity for test+merge+release. However I think spending time doing something like that is only deferring the real work to another day. If a worse security vulnerability comes along, the same thing could happen again. |
We have discussed this and agreed that we want to support Node.js LTS and current versions in general and that dropping older Node.js versions should not be considered to be a breaking change. I think it was good to raise the issue (but it wasn't me who had the extra work due to the misunderstanding, so I'm only talking for myself here). But this topic would have come up anyway at some point and at least now, we have a common point-of-view towards this. It will be documented in the README as well, when #1671 will be merged. So, even if the solution is not the one you wished for, some good has come out of it. |
@dougwilson you don't need to apologize for raising the issue! As @nknapp pointed out, had you not raised it, we wouldn't have clarified our policy. Hopefully clarifying the policy will help a future person like you avoid some frustration. Seriously, great thanks for bringing up your real-world pain and helping us work through it. |
I would recommend against treating bumping the supported Node.js runtime anything but a breaking change. FWIW, Node.js does not do this. |
The conversation is a bit confusing now since @dougwilson removed his messages, but I want to also add that the inability to both bump the dependencies to a version without the EDIT: to clarify my opinion here, I mean stop reporting on the CVE. If that is by recommending folks like Doug use an exclude in their projects, or if we can get it removed from npm audit results, then go ahead and keep old node version support in this major line. I am not trying to deny that the issues should be patched in the libraries, just that the reporting here causes more harm than good. |
It's a bit impossible to adequately weigh in on this issue but from the conversation it would appear to be a combination of issues stemming from the To be honest, at this point, I'd be very happy for the CVE for |
The CVE for minimist is hugely disruptive. It has flagged almost every single package I maintain whilst (to the best of my knowledge so far) being exploitable in none of them. |
Is this a genuine possibility? |
I don't know. It is worth looking into. |
@isaacs or @darcyclarke, is there something we can do on the registry reporting side here? Assuming that revoking the actual CVE is not an option (as it is probably valid), can we stop reporting on it in |
@wesleytodd we've got an "Open RFC - Deep Dive" call queued up for today at 2pm ET about "resolutions". I imagine the conversation will touch on the existing proposal for us to support resolving/ignoring audit notices as well as the broader topic of resolving peer/unlisted deps. I encourage folks to join the call if their interested or just want to sit in. Circling back to the question speicifically though, no I don't think there's anything we'd do on the registry side to revoke a valid CVE. Again, I believe a better way forward would be to provide a mechanism for the community - especially maintainers - to ignore/whitelist their usage of a dep against unnecessary noise. |
This particular CVE, combined with the fact that mkdirp 0.5.1's dep on minimist was pinned to an unpatched version, and that mkdirp is so widely used, highlighted a shortcoming in the current The good news is that Another aspect to this is that One thing we are considering is actually creating an advisory against mkdirp |
Since this is dying down now, and is fixed in this library via #1672, it seems fine to forego more conversation here. In the future hopefully the changes in |
@wesleytodd totally get not carrying on the conversation here, but I'm still fighting the minimist CVE across countless packages, and so would be interested to know where I can follow along with what happens with this. |
No description provided.
The text was updated successfully, but these errors were encountered: