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

[Deps] upgrade tsconfig-paths to ^4.0.0 #2447

Closed
wants to merge 1 commit into from

Conversation

F3n67u
Copy link

@F3n67u F3n67u commented May 4, 2022

This deps upgrade will introduce a breaking change, because tsconfig-paths upgrade its json5 version to v2 which does not support node 4. see dividab/tsconfig-paths#198

If 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?

@F3n67u F3n67u changed the title chore(deps): upgrade tsconfig-paths to ^4.0.0 [Deps] upgrade tsconfig-paths to ^4.0.0 May 4, 2022
@codecov
Copy link

codecov bot commented May 4, 2022

Codecov Report

Merging #2447 (70a32ba) into main (b2f6ac8) will not change coverage.
The diff coverage is n/a.

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2f6ac8...70a32ba. Read the comment docs.

Copy link

@MichaelDeBoey MichaelDeBoey left a 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.

@F3n67u F3n67u marked this pull request as ready for review May 4, 2022 14:22
@ljharb
Copy link
Member

ljharb commented May 4, 2022

I don’t have any interest in dropping node anything, and EOL status is irrelevant.

@MichaelDeBoey
Copy link

@ljharb It would be nice to be able to use tsconfig-paths' new features (especially supporting jsconfig.json) though, so how would you propose we should go forward in getting that done?

@ljharb
Copy link
Member

ljharb commented May 4, 2022

@MichaelDeBoey what tooling besides TS supports jsconfig.json?

@MichaelDeBoey
Copy link

MichaelDeBoey commented May 4, 2022

@ljharb We're supporting it in the @remix-run repo to have a jsconfig.json file.
Updating the codebase to use tsconfig-paths v4 to do this instead of our custom solution breaks our tests (see remix-run/remix#3074) because eslint-plugin-import & eslint-import-resolver-typescript aren't updated (yet).

import-js/eslint-import-resolver-typescript#104 was already merged & dropped supporting Node v4, I think it's fair for eslint-plugin-import to do the same, as Node v4 is really old and really long EOL.

If we merge this PR, this would be a major release.
People who still want to use Node v4 (they should have updated a long time ago imo) can still use the current major version if they really want to stick with Node v4.

Newest ESLint versions already dropped support for Node 10 and below, so that shouldn't be a problem either.

@nied
Copy link

nied commented May 5, 2022

v4 of tsconfig-paths also updates json5 to 2.2.1 which drops a version of minimist that contains a security issue: dividab/tsconfig-paths#198. We're currently updating all our dependencies to remove the vulnerable version of minimist and this package is on the list. Let me know if we can help in any way to get this merged!

@ljharb
Copy link
Member

ljharb commented May 5, 2022

@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.

@kentcdodds
Copy link
Contributor

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?

@ljharb
Copy link
Member

ljharb commented May 5, 2022

@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 ¯\_(ツ)_/¯

@MichaelDeBoey
Copy link

MichaelDeBoey commented May 5, 2022

@ljharb It could indeed be in a major release, but they opted not to do it for the reasons I said above:

  • Node v4 is EOL a long time, so people should have updated a long time ago
  • People who still want to use older Node versions could stay at the older version of tsconfig-paths

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).
All dependencies could be up to date + we're "pushing" the community towards newer Node versions, which is a good thing imo (and also a bit the "responsibility" of package maintainers).

@ljharb
Copy link
Member

ljharb commented May 5, 2022

People who still want to use older Node versions could stay at the older version of tsconfig-paths

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.

@kentcdodds
Copy link
Contributor

Can you help me understand who specifically would be harmed by dropping Node v4 support in this package? They would have to both:

  1. Be using Node v4
  2. Need to upgrade their version of eslint-plugin-import

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.

@ljharb
Copy link
Member

ljharb commented May 6, 2022

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 ecosystem

In 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 forced

Dropping 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 dependencies

This 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.

@ljharb
Copy link
Member

ljharb commented May 6, 2022

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)

@nied
Copy link

nied commented May 9, 2022

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

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:

  1. Merge this PR
  2. If you desperately want to keep Node 4 support, decide that you want to drop the tsconfig-paths dependency altogether.

@ljharb
Copy link
Member

ljharb commented May 9, 2022

@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).

@kentcdodds
Copy link
Contributor

Looks like we're not going to convince you. Bummer.

/me unsubscribes 🤷‍♂️

@ljharb
Copy link
Member

ljharb commented May 9, 2022

@kentcdodds certainly not if you're unwilling to answer #2447 (comment) ¯\_(ツ)_/¯

@cmawhorter
Copy link

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 [...]

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.

Dropping support for an EOL version of node, or a browser, doesn't help anybody

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?

@ljharb
Copy link
Member

ljharb commented May 15, 2022

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?

@nocive

This comment was marked as spam.

@Sparticuz
Copy link

It's not just the cve. This package doesn't even work if you use an extends array in your tsconfig.

@ljharb
Copy link
Member

ljharb commented Jul 27, 2023

@Sparticuz yep, unfortunately that feature wasn't backported to v3 either.

@ptrxyz

This comment was marked as off-topic.

@coreyward

This comment was marked as off-topic.

coreyward added a commit to coreyward/sanity-pills that referenced this pull request Sep 1, 2023
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
@NutchapolSal
Copy link

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.

@domdomegg
Copy link

domdomegg commented Dec 10, 2023

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

@coreyward
Copy link

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.

@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 import-js—changes to dependencies can change the code exercised in a sub-dependency. Asserting that the risk is tolerable is sometimes okay, but when an alternative package by a similar name is available and well maintained the position is rather unpalatable. The only real move for those of us working on commercial projects is to adopt the alternative package.

@ljharb
Copy link
Member

ljharb commented Dec 13, 2023

@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.

@ljharb ljharb closed this in 48fec35 Dec 14, 2023
@ljharb
Copy link
Member

ljharb commented Dec 14, 2023

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.

@import-js import-js locked as resolved and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.