[Deps] upgrade tsconfig-paths to ^4.0.0

[F3n67u]

This deps upgrade will introduce a breaking change, because tsconfig-paths upgrade its json5 version to v2 which does not support node 4. see https://github.com/dividab/tsconfig-paths/pull/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?

[codecov[bot]]

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.

[ljharb]

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

[MichaelDeBoey]

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

@MichaelDeBoey what tooling besides TS supports jsconfig.json?

[MichaelDeBoey]

@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 https://github.com/remix-run/remix/pull/3074) because eslint-plugin-import & eslint-import-resolver-typescript aren't updated (yet).

https://github.com/alexgorbatchev/eslint-import-resolver-typescript/pull/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]

v4 of tsconfig-paths also updates json5 to 2.2.1 which drops a version of minimist that contains a security issue: https://github.com/dividab/tsconfig-paths/pull/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]

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

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]

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

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

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]

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]

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]

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]

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 https://github.com/advisories/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]

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

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

/me unsubscribes 🤷‍♂️

[ljharb]

@kentcdodds certainly not if you're unwilling to answer https://github.com/import-js/eslint-plugin-import/pull/2447#issuecomment-1119864544 ¯\_(ツ)_/¯

[cmawhorter]

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]

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?

[cmawhorter]

It’s not vulnerable code, it’s a CLI arg parser that isn’t invoked with user input by this plugin

if this package was the only dependency used i'd agree that this is def not critical, but consider an app that has two dependencies:

• one is this package, with the vulnerable minimist version (as you've said, minimist is not used)
• but, another uses minimist@^1.x, but does use it

since the problem minimist version satisfies both dependencies, npm can dedupe it into the vulnerable one, and your app is hit.

so just because this package isn't an issue doesn't mean that it's problems won't bleed elsewhere.

Why does there have to be a line somewhere?

does eslint-plugin-import support node v0.10? there is always a line somewhere, and it's figuring out where that line should be. you've stated your reasoning and i'm certainly not going to fork this, so i'll just thank you for your work and leave it at that.

[ljharb]

In that case you’d just depend directly on a safe range of minimist and it’d work - or, that other package would update it.

It technically should support node 0.10 because eslint 2 does, and we support eslint 2, but we’ve never prioritized the work to do it.

[cmawhorter]

or, that other package would update it

but what if that other package is part of a supply chain attack that is designed to work in conjunction with this package?

On Sun, May 15, 2022 at 12:08 PM Jordan Harband @.***> wrote:

In that case you’d just depend directly on a safe range of minimist and it’d work - or, that other package would update it.

It technically should support node 0.10 because eslint 2 does, and we support eslint 2, but we’ve never prioritized the work to do it.

— Reply to this email directly, view it on GitHub https://github.com/import-js/eslint-plugin-import/pull/2447#issuecomment-1126998376, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABCYAVDRR4WMXYLHWINSDDVKFDTBANCNFSM5VBYRVSA . You are receiving this because you commented.Message ID: @.***>

[ljharb]

You can continue contriving scenarios, but it’s never an unrelated package’s problem to fix. npm’s deduping doesn’t ever make this plugin at all responsible for another package’s vulnerability.

[ljharb]

At this point, there’s only three viable paths forward:

1. tsconfig-paths backports a fix in v3, which they really should be doing regardless
2. this PR rots and we stay on v3 forever, or until something is actually worth a breaking change
3. someone can explain why jsconfig.json is a compelling feature, as opposed to a harmful xkcd 927

[01taylop]

So will this change never be merged? I am getting frustrated because we use this dependency in an internal linting tool which is used by hundreds of projects within our company. That's a lot of Dependabot alerts to dismiss and as much as I enjoy this plugin and am grateful for it, am feeling quite let down.

[ljharb]

@01taylop nobody's tried to answer this question yet: https://github.com/import-js/eslint-plugin-import/pull/2447#issuecomment-1119864544

[01taylop]

Using a jsconfig.json is a useful tool in non-Typescript projects when using Webpack Aliases or when using Module Federation.

For example in one project we have a bi-directional shell and the jsconfig.json helps resolve imports from the host's remote.

Take the following ModuleFederationPlugin snippet for example:

new ModuleFederationPlugin({
  name: 'fooRemote',
  library: { type: 'var', name: 'fooRemote' },
  filename: 'remoteEntry.js',
  exposes: {
    './Bar': './src/js/Bar',
  },
  remotes: {
    fooRemote: 'fooRemote',
  },

The project shares the Bar component in the fooRemote remote, but also uses the Bar component itself, imported like so:

import Bar from 'fooRemote/Bar'

This is a simplified example and the project I am working on shares many different components including React context. To aid development we have configured some of the import lint rules a little differently:

'import/no-unresolved': [2, { ignore: ['^fooRemote'] }],
'import/order': [2, {
    'alphabetize': {
      order: 'asc',
    },
    'groups': ['builtin', 'external', 'internal', 'parent', 'sibling', 'index', 'object', 'type'],
    'newlines-between': 'always',
    'pathGroups': [{
      group: 'internal',
      pattern: 'fooRemote/**',
      position: 'after',
    }],
    'pathGroupsExcludedImportTypes': [],
  }],

As you can see, we have to exclude the fooRemote as it is not a resolved path, and we also adjust the import order to group the remote imports together.

It would be wonderful to support jsconfig.json so that aliases could be resolved, and there could even be an order group for aliases.

[ljharb]

Thanks, that's helpful. Both of those are antipatterns imo, but they are likely to be common needs, and unfortunately at a certain level of usage even bad practices sometimes need to be supported. I'll think on this.

[JounQin]

I searched tsconfig-paths in this repository, and can only find it used in

https://github.com/import-js/eslint-plugin-import/blob/b2f6ac8eedac22a241f9d295bcdd4eef4e1c85cf/src/ExportMap.js#L526-L550

It is using tsconfig-paths for searching tsconfig files and then require('typescript') which is very expensive, I would suggest adding a new option esModuleInterop: boolean for this plugin instead of relying on tsconfig-paths or typescript at all. And also @typescript-eslint/eslint-parser does not use tsconfig-paths, so maybe @bradzacher can help to find a better way to get if esModuleInterop is configured without tsconfig-paths.