Skip to content

Proposal for new rule - consistent style for type-only exports #3597

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
Closed
leepowelldev opened this issue Jun 30, 2021 · 13 comments
Closed

Proposal for new rule - consistent style for type-only exports #3597

leepowelldev opened this issue Jun 30, 2021 · 13 comments
Labels
enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@leepowelldev
Copy link

leepowelldev commented Jun 30, 2021 •
edited by bradzacher
Loading

Basically this is the export version of this proposal which has now been implemented:
#2200

Proposal

{
  "rules": {
    "@typescript-eslint/prefer-export-type": ["error"]
  }
}
// your repro code case
type Foo = string;
type Bar = number;

function Component() {
  // ...
}

export {.Component }
// No error
export type {.Foo }
// Error
export { Bar }

This is useful for having alongside import/exports-last rule: https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/exports-last.md
@leepowelldev leepowelldev added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Jun 30, 2021
@bradzacher
Copy link
Member

I'll leave this open, as it is a code style lint rule, but IMO it's pretty low value (see below). If someone wants to work on it - they can feel free to.

I definitely get the motivation - but I don't think this really makes much of a difference in code.

consistent-type-imports matters because it allows transpilers to understand side-effects and optimise the code away at an isolated-modules level, which improves correctness.
Additionally consistent-type-imports operates solely on the AST without type-information.

Deeper explanation 
import { T } from 'module';

const x: T = 'foo';

if the build tool notices T is only used in a type location, it can scrub that usage.
However it's unknown if importing module has a side-effect, so scrubbing the entire import is somewhat unsafe to do.

OTOH, import type { T } from 'module' is known to be a type-only import, thus it has no runtime side-effects and is safe to delete at build time.

This rule would just be a coding style thing. Exports for types declared in the same file can be statically verified by build tools to be deletable. All other exports are either values or require type information to understand.

Deeper explanation 
type T = string;
export { T };

Single-file static analysis tells us T is a type, so we know we can delete this at build time

export type { T } would save us needing single-file static analysis to determine it, but ultimately makes no difference because the analysis is done ahead of time on the file, so you're really only saving a close to O(1) lookup in the data-structures.

@bradzacher bradzacher added enhancement: new plugin rule New rule request for eslint-plugin and removed triage Waiting for team members to take a look labels Jul 6, 2021
@AprilArcus
Copy link

AprilArcus commented Jul 13, 2021 •
edited
Loading

webpack balks at type-only exports when used with babel:

babel/babel-loader#603

@Belco90
Copy link

Belco90 commented Aug 24, 2021

This rule would be great to force type-only re-exports indeed.

You can find an example in this issue from Storybook storybookjs/storybook#13504

@millsp
Copy link

millsp commented Sep 13, 2021 •
edited
Loading

Useful for esbuild as well which currently fails (esm) because of an export that is re-exporting an import type, and that leads to a "module not found".

@millsp
Copy link

millsp commented Sep 14, 2021

I could get over this problem by enabling isolatedModules in tsconfig, which was the intent all along. Now TS checks for that instead.

@bradzacher
Copy link
Member

bradzacher commented Sep 14, 2021 •
edited
Loading

edit I thought that isolatedModules will cause TS to typecheck each file as if it were the only file in the project. All its imports will be treated as any. Which means that most of the time it won't catch any type errors.

I was wrong.

@millsp
Copy link

millsp commented Sep 14, 2021

I don't think that is the case anymore, by reading their docs https://www.typescriptlang.org/tsconfig#isolatedModules

It does not change the behavior of your code, or otherwise change the behavior of TypeScript’s checking and emitting process.

I can confirm that type-checks happen as expected on my local projects.

@dzearing
Copy link
Contributor

I would greatly appreciate this rule. I had considered isolatedModules as the fix here, but the nice thing about linting is autofixing, so I would be able to bulk fix this in thousands packages that currently violate this.

Would a PR be welcome? Or is there someone working on this?

For background, the particular scenario that we hit looks like this:

A library exports a type from another library:

export { ButtonProps } from 'component-library';

A tool like esbuild is used to transpile the TypeScript and produce an ESM bundle of the library to be consumed by a browser, externalizing all dependencies (which will be resolved with import maps.) Because the export is not explicitly a type, and esbuild doesn't know the types of component-library, the export is not dropped from output and therefore the esm output fails to resolve the missing type when loaded in the browser.

If the type export is explicit about it being a type, esbuild can drop the export safely and the bundle becomes browser consumable.

@bradzacher
Copy link
Member

We are a community run project. The volunteer maintainers spend most of their time triaging issues and reviewing PRs. This means that most issues will not progress unless a member of the community steps up and champions it.

If this issue is important to you - consider being that champion.
If not - please use the subscribe function and wait patiently for someone else to implement.

@dzearing
Copy link
Contributor

Cool beans. I'll get a PR going.

@dzearing dzearing mentioned this issue Sep 28, 2021
Merged
@dzearing
Copy link
Contributor

This is now in PR. Also I just realized Andrew merged this in:

microsoft/TypeScript#45998

Which allows for type annotations inside brackets next to specifiers:

import { Button, type ButtonProps } from '...';
export { Button, type ButtonProps } from '...';

So this might be addressed in a future PR to add an option to prefer-inline-types for both import and export rules, which should only be enabled for TS 4.5+ projects. I'm wondering now if there are rules which are TS version specific, and how to specify minimum version requirements.

@markjm
Copy link

markjm commented Sep 30, 2021

Thanks @dzearing for looking at this. Also worth noting that enabling this rule may have positive perf implications for ts-loader/webpack usage as well

webpack/webpack#14367 (comment)

@JoshuaKGoldberg
Copy link
Member

Resolved by #3936. Thanks again @dzearing! 🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@leepowelldev @dzearing @AprilArcus @Belco90 @JoshuaKGoldberg @bradzacher @markjm @millsp