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

[New] extensions: add the checkTypeImports option #2817

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

phryneas
Copy link
Contributor

@phryneas phryneas commented Jul 5, 2023

Currently, the patterns

import type { Foo } from './foo';

export type { Foo } from './foo';

are ignored by the import/extensions rule.

This adds the checkTypeImports option to that rule, so these types of imports can be checked on an opt-in basis.

@ljharb
Copy link
Member

ljharb commented Jul 28, 2023

Is this something that's more than purely style? At runtime, the types are erased, so does it matter what extensions are in the specifier?

@phryneas
Copy link
Contributor Author

If you author a library, it is vital. If your types are missing the file extension (or have a .ts instead of a .js extension), consumers with the tsconfig setting moduleResolution: "node16" will not be able to use your library.

@ljharb
Copy link
Member

ljharb commented Jul 28, 2023

That seems like a flaw in TS's moduleResolution then - it's not reasonable to expect your entire dep graph to update its specifier style just to make types work.

@phryneas
Copy link
Contributor Author

phryneas commented Jul 28, 2023

Honestly: can we please not have this discussion about something we cannot change? In the end, import type is something the TS team invented, and this is how they decided it should work.

This is behaviour of TypeScript at least from version 4.7 (where node16 was introduced) to 5.2 (current beta), and even if TypeScript would suddenly change their mind, as library authors we need to write code that works with these TypeScript versions, too.

As you can see here, a library omitting the extension in their types will end up with an "Internal resolution error (2)" if consumed in ESM mode. here you can see that we fixed it. This is the PR that we did for that, and we would like to use this ESLint plugin to make sure that we keep our code compliant - isn't that pretty much the purpose of this plugin?

This PR just adds another option - it's not even on by default 😕

@ljharb
Copy link
Member

ljharb commented Jul 28, 2023

That seems reasonable, but it's still worth pursuing a fix in TS so that the lint rule isn't needed in the long run - have you filed an issue there?

@phryneas
Copy link
Contributor Author

phryneas commented Jul 28, 2023

There is ongoing discussion in many issues - I think their general argument is that it's required for all other imports to stay ESM-compatible, so it should for those, too.

There is movement to allow it together with allowImportingTsExtensions over in microsoft/TypeScript#54746, but I think it's more about .ts instead of .js, not about no extension at all. There is a lot of context in this gist by Andrew Branch, and I'm very sure they thought about that more than just twice.

Keep in mind, this is not about CJS packages being consumed by ESM, but about how the types of ESM packages consumed by ESM should look like, so they are striving for some kind of new standard here either way and "old ecusystem" won't be too affected by it, since that never shipped ESM in the first place.

@ljharb
Copy link
Member

ljharb commented Jul 28, 2023

That's fine for first-party code, but requiring third-party packages to change how they do things so it works with a new system is exactly why native ESM has had such paltry adoption so far.

@phryneas
Copy link
Contributor Author

🤷‍♂️ Please don't shoot the messenger. 😕

docs/rules/extensions.md Outdated Show resolved Hide resolved
src/rules/extensions.js Outdated Show resolved Hide resolved
tests/src/rules/extensions.js Show resolved Hide resolved
@phryneas
Copy link
Contributor Author

phryneas commented Aug 2, 2023

Looking at those "opposite" tests, this will need to have #2813 merged in first so I can use ruleTesterWithTypeScriptImports in the tests - right now it does not resolve extensions correctly for .ts, so the { ts: 'always', tsx: 'always', js: 'always', jsx: 'always', checkTypeImports: true } line pretty much boils down to { checkTypeImports: true } for imports without extension.

I'll wait for that other PR and then rework the test for this one to include the correct parser.

@ljharb
Copy link
Member

ljharb commented Aug 5, 2023

The ruleTesterWithTypeScriptImports part is pretty simple; I think you can pull that into this PR. If you leave it in a separate commit I can preserve authorship.

@phryneas
Copy link
Contributor Author

I've added the comment to the doc you requested and some additional tests with ruleTesterWithTypeScriptImports.

As for code ownership - honestly, I don't care much about that, I have too many open PRs. Feel free to just edit away in this :)

@phryneas
Copy link
Contributor Author

Seems like the CI errors are unrelated to this PR.

@alexgwolff
Copy link

Hi, any update about this?

@luisfernandomoraes
Copy link

The checkTypeImports option is a great idea. It ensures that type imports get the same extension checks as regular imports, which is super useful for keeping code consistent and clean, especially in TypeScript projects with strict settings.

@ljharb ljharb force-pushed the pr/extensions-checkTypeImports branch from a3e5f23 to d225176 Compare October 3, 2024 04:34
@ljharb ljharb changed the title extensions: add the checkTypeImports option [New] extensions: add the checkTypeImports option Oct 3, 2024
@ljharb ljharb merged commit d225176 into import-js:main Oct 3, 2024
319 checks passed
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 4, 2024
| datasource | package              | from   | to     |
| ---------- | -------------------- | ------ | ------ |
| npm        | eslint-plugin-import | 2.30.0 | 2.31.0 |


## [v2.31.0](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#2310---2024-10-03)

##### Added

-   support eslint v9 (\[[#2996](import-js/eslint-plugin-import#2996)], thanks \[[@G-Rath](https://github.com/G-Rath)] \[[@michaelfaith](https://github.com/michaelfaith)])
-   \[`order`]: allow validating named imports (\[[#3043](import-js/eslint-plugin-import#3043)], thanks \[[@manuth](https://github.com/manuth)])
-   \[`extensions`]: add the `checkTypeImports` option (\[[#2817](import-js/eslint-plugin-import#2817)], thanks \[[@phryneas](https://github.com/phryneas)])

##### Fixed

-   `ExportMap` / flat config: include `languageOptions` in context (\[[#3052](import-js/eslint-plugin-import#3052)], thanks \[[@michaelfaith](https://github.com/michaelfaith)])
-   \[`no-named-as-default`]: Allow using an identifier if the export is both a named and a default export (\[[#3032](import-js/eslint-plugin-import#3032)], thanks \[[@akwodkiewicz](https://github.com/akwodkiewicz)])
-   \[`export`]: False positive for exported overloaded functions in TS (\[[#3065](import-js/eslint-plugin-import#3065)], thanks \[[@liuxingbaoyu](https://github.com/liuxingbaoyu)])
-   `exportMap`: export map cache is tainted by unreliable parse results (\[[#3062](import-js/eslint-plugin-import#3062)], thanks \[[@michaelfaith](https://github.com/michaelfaith)])
-   `exportMap`: improve cacheKey when using flat config (\[[#3072](import-js/eslint-plugin-import#3072)], thanks \[[@michaelfaith](https://github.com/michaelfaith)])
-   adjust "is source type module" checks for flat config (\[[#2996](import-js/eslint-plugin-import#2996)], thanks \[[@G-Rath](https://github.com/G-Rath)])

##### Changed

-   \[Docs] \[`no-relative-packages`]: fix typo (\[[#3066](import-js/eslint-plugin-import#3066)], thanks \[[@joshuaobrien](https://github.com/joshuaobrien)])
-   \[Performance] \[`no-cycle`]: dont scc for each linted file (\[[#3068](import-js/eslint-plugin-import#3068)], thanks \[[@soryy708](https://github.com/soryy708)])
-   \[Docs] \[`no-cycle`]: add `disableScc` to docs (\[[#3070](import-js/eslint-plugin-import#3070)], thanks \[[@soryy708](https://github.com/soryy708)])
-   \[Tests] use re-exported `RuleTester` (\[[#3071](import-js/eslint-plugin-import#3071)], thanks \[[@G-Rath](https://github.com/G-Rath)])
-   \[Docs] \[`no-restricted-paths`]: fix grammar (\[[#3073](import-js/eslint-plugin-import#3073)], thanks \[[@unbeauvoyage](https://github.com/unbeauvoyage)])
-   \[Tests] \[`no-default-export`], \[`no-named-export`]:  add test case (thanks \[[@G-Rath](https://github.com/G-Rath)])
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 4, 2024
| datasource | package              | from   | to     |
| ---------- | -------------------- | ------ | ------ |
| npm        | eslint-plugin-import | 2.30.0 | 2.31.0 |


## [v2.31.0](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#2310---2024-10-03)

##### Added

-   support eslint v9 (\[[#2996](import-js/eslint-plugin-import#2996)], thanks \[[@G-Rath](https://github.com/G-Rath)] \[[@michaelfaith](https://github.com/michaelfaith)])
-   \[`order`]: allow validating named imports (\[[#3043](import-js/eslint-plugin-import#3043)], thanks \[[@manuth](https://github.com/manuth)])
-   \[`extensions`]: add the `checkTypeImports` option (\[[#2817](import-js/eslint-plugin-import#2817)], thanks \[[@phryneas](https://github.com/phryneas)])

##### Fixed

-   `ExportMap` / flat config: include `languageOptions` in context (\[[#3052](import-js/eslint-plugin-import#3052)], thanks \[[@michaelfaith](https://github.com/michaelfaith)])
-   \[`no-named-as-default`]: Allow using an identifier if the export is both a named and a default export (\[[#3032](import-js/eslint-plugin-import#3032)], thanks \[[@akwodkiewicz](https://github.com/akwodkiewicz)])
-   \[`export`]: False positive for exported overloaded functions in TS (\[[#3065](import-js/eslint-plugin-import#3065)], thanks \[[@liuxingbaoyu](https://github.com/liuxingbaoyu)])
-   `exportMap`: export map cache is tainted by unreliable parse results (\[[#3062](import-js/eslint-plugin-import#3062)], thanks \[[@michaelfaith](https://github.com/michaelfaith)])
-   `exportMap`: improve cacheKey when using flat config (\[[#3072](import-js/eslint-plugin-import#3072)], thanks \[[@michaelfaith](https://github.com/michaelfaith)])
-   adjust "is source type module" checks for flat config (\[[#2996](import-js/eslint-plugin-import#2996)], thanks \[[@G-Rath](https://github.com/G-Rath)])

##### Changed

-   \[Docs] \[`no-relative-packages`]: fix typo (\[[#3066](import-js/eslint-plugin-import#3066)], thanks \[[@joshuaobrien](https://github.com/joshuaobrien)])
-   \[Performance] \[`no-cycle`]: dont scc for each linted file (\[[#3068](import-js/eslint-plugin-import#3068)], thanks \[[@soryy708](https://github.com/soryy708)])
-   \[Docs] \[`no-cycle`]: add `disableScc` to docs (\[[#3070](import-js/eslint-plugin-import#3070)], thanks \[[@soryy708](https://github.com/soryy708)])
-   \[Tests] use re-exported `RuleTester` (\[[#3071](import-js/eslint-plugin-import#3071)], thanks \[[@G-Rath](https://github.com/G-Rath)])
-   \[Docs] \[`no-restricted-paths`]: fix grammar (\[[#3073](import-js/eslint-plugin-import#3073)], thanks \[[@unbeauvoyage](https://github.com/unbeauvoyage)])
-   \[Tests] \[`no-default-export`], \[`no-named-export`]:  add test case (thanks \[[@G-Rath](https://github.com/G-Rath)])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants