-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[Deps] upgrade tsconfig-paths to ^4.0.0 #2447
Conversation
0ba4803
to
70a32ba
Compare
Codecov Report
@@ Coverage Diff @@
## main #2447 +/- ##
=======================================
Coverage 95.13% 95.13%
=======================================
Files 66 66
Lines 2754 2754
Branches 927 927
=======================================
Hits 2620 2620
Misses 134 134 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fair to drop Node v4 support as it's been EOL for a long time now.
I don’t have any interest in dropping node anything, and EOL status is irrelevant. |
@ljharb It would be nice to be able to use |
@MichaelDeBoey what tooling besides TS supports |
@ljharb We're supporting it in the @remix-run repo to have a import-js/eslint-import-resolver-typescript#104 was already merged & dropped supporting Node v4, I think it's fair for If we merge this PR, this would be a major release. Newest ESLint versions already dropped support for Node 10 and below, so that shouldn't be a problem either. |
v4 of |
@nied a) that "security issue" is not one, because it's a self-attack; b) tsconfig-paths v3 depends via semver range on a version of minimist that is NOT vulnerable, so that motivation is nonexistent. |
Can you help me understand why you're willing to block actual who are actively working on projects and need this change so that you don't break theoretical people using Node v4 who (if they exist at all) are probably not actively working on projects and could stay with an old version of the package? |
@kentcdodds you could ask the same question to the tsconfig-paths maintainers who opted to do a breaking change instead of adding the feature you need in a v3 minor ¯\_(ツ)_/¯ |
@ljharb It could indeed be in a major release, but they opted not to do it for the reasons I said above:
I really think dropping this old Node version should be a no brainer as we're already at v18 now (v16 if we're talking about a stable version). |
no, we (this plugin) can't, because we want the new feature and also want to use node 4. It is wildly inappropriate and harmful for project maintainers to try to "push" people to upgrade. Folks who haven't upgraded can't; nobody needs the push. |
Can you help me understand who specifically would be harmed by dropping Node v4 support in this package? They would have to both:
Can you give examples of specific people who fall into this category? I can give you examples of specific people who are harmed by not pursuing this. My personal expectation is that anyone on Node v4 is already locked-in on their dependencies and not upgrading anything anyway. |
There seems to be multiple issues, needs, and philosophical concerns here, so let me type a mini-essay to summarize: Breaking changes in packages are harmful for the ecosystemIn general, when a package (something that can be used transitively, which notably does NOT really include eslint plugins) has a breaking change, that causes a number of very harmful and costly downstream effects. It means that consumers have to choose whether to make their own breaking change, or be stuck on an older major; it means that if there's ever a CVE on that package, consumers transitively depending on the old major are far more likely to be stuck on it. Additionally, there's a number of popular packages/maintainers that do unnecessary major bumps - iow, the major bump is correct, but the actual breaking thing was unnecessary. For example, dropping a node version or using new syntax when there's not a strong reason to do so. In many cases, the harm and cost of these unnecessary major bumps is worse than if the package had never existed - in other words, it can cancel out the lifetime benefit of the package itself. As such, I have developed an intense avoidance for breaking changes in all of my packages, because I don't want to inflict hundreds of millions of dollars of person-hour cost on the entire industry unnecessarily. dropping platforms does not force change, and change should not be forcedDropping support for an EOL version of node, or a browser, doesn't help anybody. Nobody stays on an old version of node or a browser by choice, they do it because they have no other choice. The reasons might be inexperience, or lack of resources, or all sorts of things - but the reasons are irrelevant. Punishing these users doesn't help them upgrade, it just disrupts their roadmap by forcing them to deprioritize things that might be what's keeping their business alive, and instead to upgrade. Whether that's a better outcome or not is not your place to decide. If you need to drop a platform because of a compelling reason, great! That's what semver-major is for. Just please don't pretend there's some kind of positive effect - it's a large negative effect, that is hopefully outweighed by whatever motivation you have for the major bump. People on old platforms don't update dependenciesThis expectation is simply false; they update everything they can, it's just that deps that do the hostile things described above prevent them from doing so. "they can just use an old version"No, they can't, for a ton of reasons: vulnerabilities, often in transitive dependencies; bugfixes; new features. Forcing them to update node just to get a new feature in "not node" makes no sense, unless that new feature requires functionality only present in newer node. |
Now, let's speak to this specific PR. If a feature is compelling enough, especially if it actually requires a newer version of node (this one surely does not, except that tsconfig-paths dropped support), then it might be worth the breaking change. However, jsconfig.json does not feel compelling to me. TS users use tsconfig.json; .editorconfig is a universal standard; but jsconfig.json is an upstart format that is not widely supported. Tools supporting a new format can be helpful when it's low-cost, but as described above, a major change is costly. Separately, tools supporting minimally used formats can be harmful: Webpack supported "concord modules" or something for years, adding tons of bloat and complexity for every one of its users, for a module format that virtually zero people ever cared about. Can you help me understand what about jsconfig.json is a compelling feature that a non-TS user would want? (note the dep is ts config paths, so it should only be relevant for TS users) |
You literally have an open CVE in GHSA-xvch-5gv4-984h as long as you're not merging this. By staying on Node 4, people are vulnerable to a lot of security issues that have been fixed in later Node versions (look at the list at https://www.cvedetails.com/vulnerability-list/vendor_id-12113/Nodejs.html) so keeping Node 4 support to be able to fix CVEs is kind of a moot point imho - they're already at risk. You could argue that another issue will not make the situation better, and that's fair, but this way you are actively hurting people who actually are trying to stay on top of security issues, dependabot alerts etc. I get that you want to keep backwards compatibility but at this point it's getting a bit absurd. If you care so much about the consumers of your package, surely you don't want open CVEs in your repository, right? I see two ways of moving forward here:
|
@nied that CVE isn’t one; it’s a self-attack when you craft command input to a CLI - one we’re not using at all. That CVE can remain open forever and it doesn’t matter. Thus, i never need to drop tsconfig-paths 3, nor upgrade it, because this CVE is invalid (like most CVEs in the JS ecosystem). |
Looks like we're not going to convince you. Bummer. /me unsubscribes 🤷♂️ |
@kentcdodds certainly not if you're unwilling to answer #2447 (comment) ¯\_(ツ)_/¯ |
i hear you that it's not a high priority, but i'd disagree that it's a non-issue. vuln code is vuln code and even if it's not immediately/publicly an problem doesn't mean it can't be used to escalate privileges/whatever somewhere, somehow, on some misconfigured machine out there probably holding all our medical records and built by some high school intern.
this. node's EOL has caused me pain in the ecosystem via strict adherence by maintainers. but... there has to be a line somewhere for when support for old node versions is dropped. i'd ask -- should users of newer node versions get a slap in the name of older support? |
It’s not vulnerable code, it’s a CLI arg parser that isn’t invoked with user input by this plugin. There exists no possible exploit here. So yes, it does mean it can never be used for that - at least with the same likelihood as all other code that doesn’t yet have a known vulnerability. This reaction to the mere existence of a CVE is just the FUD caused by the broken CVE system. Why does there have to be a line somewhere? |
This comment was marked as spam.
This comment was marked as spam.
It's not just the cve. This package doesn't even work if you use an extends array in your tsconfig. |
@Sparticuz yep, unfortunately that feature wasn't backported to v3 either. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This also replaces eslint-plugin-import package with eslint-plugin-i. The author of the former refuses to update dependencies that would break compatibility with Node 4—an 8 year old, unmaintained version of Node—despite security vulnerabilities that exist in them. import-js/eslint-plugin-import#2447
If you insist that you have to support Node v4, have you considered splitting releases into v2 and v3? v2 can keep support for old Node versions while v3 get updated dependencies and use new features. Bug fixes that apply to both can be merged into both lines. I think it is better than the current situation. |
If people are concerned about the CVEs (which you shouldn't be - see here), you can update json5 now there is a fix on v1. If people want support for multiple extends is tsconfig, there is a backport that would allow that here: dividab/tsconfig-paths#260 |
@domdomegg I think you missed a link ("see here"). Most corporate security policies require CVEs to be reviewed and have resolution tracked as part of the processes designed to meet security standards (SOC2, ISO 27001, NIST, etc). Determining that affected code is unused requires a fair amount of investigation, especially for common sub-dependencies like |
@coreyward I think you missed that the CVE is fixed in json5 v1 - in other words, even for corporations that have incorrectly rigid security policies, the CVE no longer applies if you update your deps, in-range. |
Since the desired functionality is now backported into tsconfig-paths v3, there's no longer any reason to upgrade to v4, and this PR is no longer needed. |
This deps upgrade will introduce a breaking change, because
tsconfig-paths
upgrade itsjson5
version to v2 which does not support node 4. see dividab/tsconfig-paths#198If we want to upgrade
tsconfig-paths
to the latest, I think we could do it on the next major release and drop the node v4 support. But I am sure if it is accepted. @ljharb what do you think?