8000 Proposal: Name components and Props once · Issue #1959 · Shopify/polaris · GitHub
[go: up one dir, main page]

Skip to content
Proposal: Name components and Props once #1959
@BPScott

Description

@BPScott

Default exports are low-key annoying as their name is "default" so every time you import them you have to give them a name. Instead of naming something once and using it consistently everywhere (and having TS shout at us if we misname it) we have to make sure we use the same name manually in every file (JS doesn't care but its would be confusing for us to have two names for the same concept).

I would like to propose that we stop using default exports and instead use named exports for everything. Additionally we should name Props based upon the component to avoid needing to rename props when used in other components to avoid naming clashes. This would save on export renaming, and would lead to simpler usage in sister components and reexporting in component indexes.

We can help enforce this by enabling the import/no-default-export eslint rule.

Further reading from someone else who came to the same conclusion: https://humanwhocodes.com/blog/2019/01/stop-using-default-exports-javascript-module/

The current world

Currently in our components we export the component as the default and its props as Props in both src/components/ComponentName/index.js and src/components/ComponentName/ComponentName.js. We then reexport with a different name in the components index. Additionally in the cases where components are used by other components we have to rename the imports as they would conflict with existing

// src/components/Button/Button.tsx
export interface Props {}
export default function Button() {}


// src/components/Button/index.ts
// In reality this is usually done a bit differently as an import then export so it is a little more convoluted
export {default, Props} from './Button';


// Components are reexported in the component index, where we need to name the default and rename the Props 
// src/components/index.ts
export {default as Button, Props as ButtonProps} from './Button';


// Components and their props are often imported in other components, where we need to name the default and rename the Props
// src/components/ResourceList/ResourceList.tsx
import Button, {Props as ButtonProps} from '../Button';

The new world

Use named exports and prefix the Props with the name of the component to avoid clashes (e.g. The Props in the Button component becomes ButtonProps).

This way there is no need to rename exports when using them

// src/components/Button/Button.tsx
export interface ButtonProps {}
export function Button() {}


// src/components/Button/index.ts
// A simple reexport of named exports, no renaming needed
export {Button, ButtonProps} from './Button';


// Components are rexported in the component index
// A simple reexport of named exports, no renaming needed
// src/components/index.ts
export {Button, ButtonProps} from './Button';


// Components and their props are often imported in other components,
// A simple import of named exports, no renaming needed
// src/components/ResourceList/ResourceList.tsx
import {Button, ButtonProps} from '../Button';

Sounds neat, how would we do this?

  1. Update the Props explorer stuff in styleguide to make it look for ComponentNameProps in addition to Props when hunting for interfaces (the real fix is to infer the interface from the exported component function but that's an LOT more work)

  2. Go rename exports and Props

  3. Enable the import/no-default-export eslint rule to check we haven't missed any default exports. I've got a little script 40 line script that can hunt for default exports and ones named Props too.

Save this as test.js and run with `node test.js`
const fs = require('fs');
const glob = require('glob');
const {parseSync: babelParse, traverse: babelTraverse} = require('@babel/core');

const isComponentFilePathRegex = /components\/([a-z0-9]+)\/(\1|index)\.tsx?/i;

const result = glob
  .sync('src/**/*.{ts,tsx}')
  //.filter((filePath) => !isComponentFilePathRegex.test(filePath))
  .map((filePath) => {
    const content = fs.readFileSync(filePath, 'utf-8');
    const ast = babelParse(content, {filename: filePath});

    const defaultDeclarations = [];
    const propsDeclarations = [];

    babelTraverse(ast, {
      ExportDefaultDeclaration(path) {
        defaultDeclarations.push(path.node.declaration.name);
      },
      ExportSpecifier(path) {
        if (path.node.exported.name === 'default') {
          defaultDeclarations.push(path.node.local.name);
        }

        if (path.node.exported.name === 'Props') {
          propsDeclarations.push(path.node.local.name);
        }
      },
    });

    return {filePath, defaultDeclarations, propsDeclarations};
  })
  .filter(
    ({defaultDeclarations, propsDeclarations}) =>
      defaultDeclarations.length > 0 || propsDeclarations.length > 0,
  );

console.log(result);

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions

    0