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

Update import/extensions rules in typescript config #2875

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

Conversation

OlivierZal
Copy link

No description provided.

@OlivierZal
Copy link
Author

OlivierZal commented Sep 4, 2023

Related to #2853

Successfully tested end-to-end.

@OlivierZal OlivierZal changed the title Update import/extensions rules in typescript config Update import/extensions rules in typescript config Sep 4, 2023

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

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

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.

@ljharb ljharb marked this pull request as draft August 30, 2024 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants