Add support @typescript-eslint/parser@6

[MillerSvt]

In v5 we had this structure for export import:

TSModuleBlock.body [
    TSImportEqualsDeclaration.isExport true
]

In v6 this structure was changed:

TSModuleBlock.body [
    ExportNamedDeclaration.declaration
        TSImportEqualsDeclaration
]

Property isExport in TSImportEqualsDeclaration was removed, and TSImportEqualsDeclaration placed into ExportNamedDeclaration.declaration instead.

To maintain the current logic, added processing of the new structure. But perhaps in the future it is worth considering equating export import to exports instead of imports.

Closes: #156

[MillerSvt]

@lydell due to exclusion some tests from node v14 (because of uncompatibility with typescript-eslint/parser@6) coverage for node 14 was decreased.

[MillerSvt]

Should we restrict max version of @typescript-eslint/parser?

[lydell]

due to exclusion some tests from node v14 (because of uncompatibility with typescript-eslint/parser@6) coverage for node 14 was decreased.

We can remove Node.js 14 from CI. While at it, we could remove Node.js 16 too (which EOL too). So we’ll just test Node.js 18 and 20.

Should we restrict max version of @typescript-eslint/parser?

I don’t think so. This is the first time there has been a compatibility issue. For the rest of the time, it’s nice not having to wait for me updating the max version.

[jakebailey]

This PR doesn't add any tests; I think it deserves a case for these statements inside of namespaces (where IMO they should not be sorted).

[lydell]

Why should they not be sorted inside of namespaces?

[jakebailey]

I'm on the fence about it...

I think that it makes some sense to do this for import = require(...) and export = ... because those are effectively the same as regular imports/exports (they just mean CJS syntax more explicitly). Though, I don't think TS's organize import command actually touches those (need to check; don't quote me), nor does dprint (which makes me a little bit unsure given I and others are still trying to get some sort of alignment between tools).

It's less clear to me that it makes sense to do this for import Foo = Some.Namespace.Thing beacuse that is not actually an import (by modern standards). It's more like a local type alias / variable assignment that you can "dot" access into to reference nested values/types (as it's still a namespace). It's only needed in super specific situations outside of namespaces.

Historically, this syntax existed pre-modules when namespaces were not called namespaces, but "internal modules". This was the only way to split files apart and reference each other, creating local variables that were just object accesses under the hood, along with being able to reference types within them.

import export Foo = Namespace.Foo... I can largely see that just being treated as a regular export. (But I also think in general that exports not in export blocks should not be reordered at all.)

So, I should really revise my statement to be that I don't think that import Foo = Namespace.Foo is likely to be a good thing to sort, in any location.

[lydell]

Can sorting them break anything?

Organize Imports in VSCode seems to move them to the bottom of an empty-line-delimited chunk of imports, but does not change their internal order. (Which was something I originally considered in #144)

[jakebailey]

Can sorting them break anything?

Sorting between them, likely not, but if you want to reexport them as values, the imports of have to be before the exports (usually...), so nothing different than regular imports/exports.

Organize Imports in VSCode seems to move them to the bottom of an empty-line-delimited chunk of imports, but does not change their internal order. (Which was something I originally considered in #144)

I haven't been able to get organize imports to touch import ... = ...; can you give the example for me to try out?

[lydell]

I made a new directory, and created index.ts with these contents:

import b from "./b"
import BC = B.C
import npm from "npm"
import AC = A.C
import a from "./a"

console.log(a, b, npm)

Then I opened VSCode in that directory (code .) and used the Organize Imports feature. Result:

import npm from "npm"
import a from "./a"
import b from "./b"
import BC = B.C
import AC = A.C

console.log(a, b, npm)

Notice how the import assignments “fell to the bottom” and maintained their internal order.

[jakebailey]

I see. I guess that's okay, then. I'm still not sure that I feel that this should also happen within namespaces (because they're not "really" imports and organize imports doesn't touch those?), but I feel better knowing that TS is doing this.

[lydell]

I still don’t understand why “inside namespace” should be a special case. Is it because “I don’t think VSCode’s Organize Imports does that”? If so, why doesn’t VSCode do it?

[jakebailey]

I don't really have any concrete reason other than that I don't feel like these are actually imports as most people know them, i.e. a part of a module system like ESM or CJS. They're an artifact of namespaces, and TS does not generally implement things like auto-imports and so on for them.

[MillerSvt]

Well, in that case, we are not satisfied with how your rule works. it should not break the sorting of our projects. I'm going to publish a fork of this rule soon.

If the parser has changed the structure, this is the parser's problem, and we should not suffer from it. I consider the decision to roll back this fix to be erroneous.

[MillerSvt]

This PR doesn't add any tests; I think it deserves a case for these statements inside of namespaces (where IMO they should not be sorted).

Have you watched it? Weren't there any of these tests? They were and worked on the 5th parser.

[lydell]

I'm going to publish a fork of this rule soon.

Please let me know once you’ve got your fork up so I can link to it!

[jakebailey]

Reverting everything was not the outcome I was expecting...

I still think sorting the CJS style imports/exports could be valuable.

Have you watched it? Weren't there any of these tests? They were and worked on the 5th parser.

I didn't see any added for the inside namespace case, which is the code that crashed. Adding the other parser tests can make sense, but it didn't seem like they were testing the same crash?

[MillerSvt]

Adding the other parser tests can make sense, but it didn't seem like they were testing the same crash?

The problem was in the parser, the same tests that were already there earlier, including tests for exporting from namespace, work fine on the fifth version, but break on the sixth. Therefore, in this case, it is not necessary to add new special test cases.

[jakebailey]

You're right, sorry. I see them now.

[jakebailey]

@lydell You are of course free to take or not take anything you want for the project, but feel pretty bad that I seemingly got this all removed. That was totally not my intention; I was just looking for consistency with tsserver and it sounded like we were converging on that...

[MillerSvt]

I'm going to publish a fork of this rule soon.

Instead, we decided to use @trivago/prettier-plugin-sort-imports.

[lydell]

There’s also https://github.com/simonhaenisch/prettier-plugin-organize-imports which I stumbled upon recently and am keeping an eye on.