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.

Sign up for GitHub

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

Jump to bottom

Update import/extensions rules in typescript config #2875

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Update import/extensions rules in typescript config #2875

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
301d371
Update import/extensions rules in typescript config
OlivierZal Sep 4, 2023
b87b288
Update typescript.js
OlivierZal Sep 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion config/typescript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,16 @@
// Omit `.d.ts` because 1) TypeScript compilation already confirms that
// types are resolved, and 2) it would mask an unresolved
// `.ts`/`.tsx`/`.js`/`.jsx` implementation.
const typeScriptExtensions = ['.ts', '.cts', '.mts', '.tsx'];
const typeScriptKeys = ['ts', 'cts', 'mts', 'tsx'];
const typeScriptExtensions = typeScriptKeys.map((key) => `.${key}`);
const typeScriptFiles = typeScriptExtensions.map((ext) => `**/*${ext}`);

const allExtensions = [...typeScriptExtensions, '.js', '.jsx'];

const typeScriptRules = Object.fromEntries(
typeScriptKeys.map((key) => [key, 'never']),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn’t this depend on your resolution settings in tsconfig?

Copy link
Author

@OlivierZal OlivierZal Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the most recent versions of TypeScript we can configure in the tsconfig if we want ts extensions to be allowed or not while importing (https://www.typescriptlang.org/tsconfig#allowImportingTsExtensions).

That's why it's good to deactivate this eslint rule for ts extensions, so the tsconfig can raise an error (or not) without conflicting with the eslint rule.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then it shouldn't be setting it to "never", should it? We don't have an "ignore" setting, so i don't think adding this makes much sense.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, but never is the default and main use of typescript (compilation), the other use is type-checking-only.

In my opinion a user setting the eslint-import-plugin typescript config wouldn't like to constantly have an eslint issue although he respects the typescript rules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - which is why the override here should be disabling the rule, not configuring it.

Copy link
Author

@OlivierZal OlivierZal Sep 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so should depend on moduleResolution

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb, and what about a new entry point typescript-node16 importing current typescript config and adding overrides to it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a new entry point may be reasonable, but calling it "node16" implies you should be using it if you're on node 16+, which isn't the case.

instead could we use tsconfig-paths to read the tsconfig and dynamically set the right thing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with but I can try. I need to have in mind that sometimes a tsconfig extends another one beforehand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but we don't support that since we're stuck on tsconfig-paths v3, and they didn't backport that feature from v4. It would be automatically handled if that changes, so you don't have to worry about it.

);

module.exports = {
settings: {
'import/extensions': allExtensions,
Expand All @@ -31,4 +37,16 @@ module.exports = {
// TypeScript compilation already ensures that named imports exist in the referenced module
'import/named': 'off',
},
overrides: [
{
files: typeScriptFiles,
rules: {
'import/extensions': [
'error',
'ignorePackages',
typeScriptRules,
],
},
},
],
};
Loading