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

feat(lint/useExportType): add rule #29

Merged
merged 2 commits into from
Dec 7, 2023
Merged

Conversation

Conaclos
Copy link
Member

@Conaclos Conaclos commented Aug 21, 2023

Summary

This rule implements some part of consistent-type-exports.
In contrast to the ESLint rule:

  • we use export { type } by default. ESLint provides an option for this and split an export between a value export and a export type.

    For example, given the following code:

    interface I {}
    class C {}
    export { I, C }

    ESlint proposes the following fix:

    interface I {}
    class C {}
    export { C }
    export type { I }

    While this rules proposes the following fix:

    interface I {}
    class C {}
    export { type I, C }
  • The ESLint rule is aware of which re-export name is a type. Biome has no information on re-exported names.

  • ESLint doesn't factorize (group) an export that contains only type-only exports.

    For example, Biome proposes the following fix, while ESLint doesn't:

    - export { type X, type Y }
    + export type { X, Y }

Test Plan

Tests included.

@Conaclos Conaclos temporarily deployed to Website deployment August 21, 2023 10:49 — with GitHub Actions Inactive
@Conaclos
Copy link
Member Author

See the ongoing discussion in rome#4751.

@github-actions github-actions bot added A-Project Area: project A-Linter Area: linter A-Parser Area: parser A-Website Area: website L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Aug 21, 2023
@Conaclos Conaclos temporarily deployed to Website deployment August 21, 2023 10:51 — with GitHub Actions Inactive
@Conaclos Conaclos added the A-Changelog Area: changelog label Aug 25, 2023
Copy link
Contributor

github-actions bot commented Nov 13, 2023

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 49701 49701 0
Passed 48721 48721 0
Failed 980 980 0
Panics 0 0 0
Coverage 98.03% 98.03% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6322 6322 0
Passed 2036 2036 0
Failed 4286 4286 0
Panics 0 0 0
Coverage 32.20% 32.20% 0.00%

ts/babel

Test result main count This PR count Difference
Total 662 662 0
Passed 592 592 0
Failed 70 70 0
Panics 0 0 0
Coverage 89.43% 89.43% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17646 17646 0
Passed 13453 13453 0
Failed 4191 4191 0
Panics 2 2 0
Coverage 76.24% 76.24% 0.00%

Copy link

netlify bot commented Dec 3, 2023

Deploy Preview for biomejs ready!

Name Link
🔨 Latest commit 53d25fb
🔍 Latest deploy log https://app.netlify.com/sites/biomejs/deploys/65720ac4fdeaa100084677ee
😎 Deploy Preview https://deploy-preview-29--biomejs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot removed the A-Changelog Area: changelog label Dec 3, 2023
@ematipico
Copy link
Member

ematipico commented Dec 4, 2023

Factorize export { type X, type Y } into export type { X, Y }

This is already done by https://biomejs.dev/linter/rules/use-grouped-type-import/ , do we need the fix in this rule too? Or, is there something I am missing?

@Conaclos
Copy link
Member Author

Conaclos commented Dec 4, 2023

Factorize export { type X, type Y } into export type { X, Y }

This is already done by https://biomejs.dev/linter/rules/use-grouped-type-import/ , do we need the fix in this rule too? Or, is there something I am missing?

useGroupedTypeImport factorize import { type } to import type {}. It doesn't factorize export { type }.

We discussed it in the corresponding rome issue.
To summarize:

We could implement two rules and deprecate useGroupedTypeImport:

  • useExportType that enforces the use of export type and export { type } and factorizes the latter into the former.
  • useImportType that enforces the use of import type and import { type } and factorizes the latter into the former.

Alternatively, we could perform the factorization of import { type } and export { type } with organizeImports. In this case, useImportType and useExportType could produce unfactorized code.

I am a bit shared about which approach to choose. We could even adopt both.

@ematipico
Copy link
Member

Sorry I made a mistake, thank you for clarifying.

I asked because today my team needed consistent-type-imports and I misread things.

Is there an issue to track useImportType?

@Conaclos
Copy link
Member Author

Conaclos commented Dec 5, 2023

Is there an issue to track useImportType?

No, once this PR merged, I will start working on.

Have you any thoughts about which approach to use?

@ematipico
Copy link
Member

Is there an issue to track useImportType?

No, once this PR merged, I will start working on.

Have you any thoughts about which approach to use?

I think the best course of action is to use a lint rule, mainly because there's a reason if TypeScript decided to do this:

import {type something} from "something.js"
import {} from "something.js";

Hence, if there's a case where users need to have this case enabled, they should be free to turn it off using a suppression comment.

@github-actions github-actions bot added the A-Changelog Area: changelog label Dec 5, 2023
@Conaclos Conaclos marked this pull request as ready for review December 5, 2023 14:47
@Conaclos
Copy link
Member Author

Conaclos commented Dec 5, 2023

Is there an issue to track useImportType?

No, once this PR merged, I will start working on.
Have you any thoughts about which approach to use?

I think the best course of action is to use a lint rule, mainly because there's a reason if TypeScript decided to do this:

import {type something} from "something.js"
import {} from "something.js";

Hence, if there's a case where users need to have this case enabled, they should be free to turn it off using a suppression comment.

I think this doesn't hold for exports. Thus, I decided to factorize type-only exports when possible. Ready for review.

@Conaclos Conaclos requested review from a team December 5, 2023 15:39

i Safe fix: Use inline type exports.

18 │ export·{·type·Interface,·type·TypeAlias,·Enum,·func·as·f,·Class·};
Copy link
Member

Choose a reason for hiding this comment

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

Can a bundler really three shake this? Or can typescript really do that?

Copy link
Member Author

@Conaclos Conaclos Dec 7, 2023

Choose a reason for hiding this comment

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

I think so. Since the export is declared as a type, the bundler/compiler knows that the entity is not used a runtime. Thus, it can remove this specific export. Otherwise, it has to check if the definition is a type. Many bundlers, such as ESbuil assume that you use the isolated module mode because they are not able to do multi-file analysis. Thus, they are not able to understand when an imported value is only a type. Requiring type-only exports and imports help them to detect types and runtime values.

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Let's 🚢 it, people want it!

@Conaclos Conaclos merged commit c0c5c8e into main Dec 7, 2023
21 checks passed
@Conaclos Conaclos deleted the conaclos/lint/useExportType branch December 7, 2023 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Diagnostic Area: diagnostocis A-Linter Area: linter A-Parser Area: parser A-Project Area: project A-Website Area: website L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants