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

Internal: Change type import styles #3725

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jackhsu978
Copy link
Member

@jackhsu978 jackhsu978 commented Aug 12, 2024

Summary

This PR enables the following ESLint rules:

  1. consistent-type-specifier-style with prefer-inline option
  2. consistent-type-imports with inline-type-imports option

TL;DR is that type keyword is now required for type imports

import { type Foo } from 'Foo';
import type Bar from 'Bar';
type T = Foo;
const x: Bar = 1;

Why?

The benefits include:

  • Avoiding Unintentional Side Effects
  • Isolated Module Transpilation, which may reduce bundle size.

Deep Dive

How to import a named export type?

BAD

// 🔴 ReactNode and Reactelement are type imports
import { ReactNode, ReactElement } from 'react';

BAD because we prefer inline type specifiers import { type A, type B } over top-level type specifier import type { A, B }.

// 🔴 Prefer inline type specifiers over top-level type specifier
import type { ReactNode, ReactElement } from 'react';

GOOD

// ✅ Inline type specifiers
import { type ReactNode, type ReactElement } from 'react';

How to import the default export type?

If we are just importing the default export type, we can either use inline type specifier

// inline type specifier
import { type default as Foo } from './Foo';

or the top-level type specifier, which is more concise:

// top-level type specifier
import type Foo from './Foo';

How to import the default export type AND a name import type?

When we import both the default export type AND a named import, we can only use the inline type specifiers like this in order to write it in one import statement:

// Use the inline type specifiers if we are also doing some named imports
import { type default as Foo, type Bar } from './Foo';

Don't: duplicated import statements

BAD

// 🔴 Duplicated import statements for the same module
import type Foo from './Foo';
import { type Bar } from './Foo';

BAD

// 🔴 Only use the top-level specifier if we only import the default export type
// DO NOT use it if we are importing any named import types.
import type Foo, { Bar } from './Foo';

BAD

// 🔴 LegacyContext is not a named export type
import { type Foo, type Bar } from './Foo';

GOOD

// ✅ This correctly imports the default export type as Foo,
// as well as the named export type named Bar
import { type default as Foo, type Bar } from './Foo';

How to import the type of an instance of a class

Suppose we have MyClass.ts

export default class MyClass {}

The default export MyClass is also the type of its instance:

import type MyClass from './MyClass';

type Props = {
  obj: MyClass;
};

How to import a value solely for its type

Suppose we have Foo.tsx

export const MY_CONST = { a: 1, b: 2 } as const;
export default function Foo({ children }: { children: string }) {
  return <div>{children}</div>;
}

And we want to import the types for MY_CONST and Foo:

import { type ComponentProps } from 'react';
import { type default as Foo, type MY_CONST } from './Foo';

type Props = {
  constKeys: keyof typeof MY_CONST; // evaluated type: 'a' | 'b'
  children: ComponentProps<typeof Foo>['chlidren']; // evaluated type: string
};

Tip

Why do we use type import even though Foo and MY_CONST are not actually types?

This is because we are only calling typeof on these values and TypeScript knows that these imports are only necessary for type checking.

If value is imported, derive type from it instead of adding type imports

Note that MyClass, Foo, and MY_CONST are imported for type checking AND runtime. In such case, type imports would be redundant since we can just derive the types from the values.

import MyClass from './MyClass';
import Foo, { MY_CONST } from './Foo';

type Props = {
  constKeys: keyof typeof MY_CONST; // evaluated type: 'a' | 'b'
  children: ComponentProps<typeof Foo>['chlidren']; // evaluated type: string
};

export default function Bar(props: Props) {
  return (
    <Foo
      onSomeEvent={() => {
        const obj: MyClass = new MyClass();
        obj.doSomething(MY_CONST[constKeys]);
      }}
    >
      {children}
    </Foo>
  );
}

@jackhsu978 jackhsu978 requested a review from a team as a code owner August 12, 2024 17:53
@jackhsu978 jackhsu978 added the patch release Patch release label Aug 12, 2024
Copy link

netlify bot commented Aug 12, 2024

Deploy Preview for gestalt ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 6e56e51
🔍 Latest deploy log https://app.netlify.com/sites/gestalt/deploys/66ba4fff7420600008ad2cec
😎 Deploy Preview https://deploy-preview-3725--gestalt.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.

@rlingineni
Copy link
Contributor

r+ but looks like tests are failing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch release Patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants