Proposal: Name components and Props once

[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);

[chloerice]

I’m with you. The components are exported for consumption as named components, so it doesn’t make sense to have a bunch default exporting of them outside of components/index.ts. There are a bunch of shared names of subcomponents like Item, but as long as those remain statics on the base component it shouldn’t be an issue.

[8eecf0d2]

I've been wondering about the best convention for exporting components within a library context (such as @shopify/polaris) and for some reason (which I cannot articulate outside of "feels good") have always preferred to use the namespace keyword:

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

// src/components/Button/index.ts
export {Button} from './Button';

// src/components/index.ts
export {Button} from './Button';

// src/components/ResourceList/ResourceList.tsx
import {Button} from '../Button';

There are rumors about the namespace keyword being deprecated however the TS core team say's otherwise while noting that they may be made redundant in a ESM world.

I'm very curious to hear your thoughts on this convention since your experiences in component library development would be much greater than my own.

[BPScott]

I am unfamiliar with namespaces. A cursory search suggests that they were born from a world prior to es modules where global variables were common, given this comment by Dan, the product manager for TS:

Namespaces imply one of two things:

1. Global code structuring (as they'll merge across files)
2. Extra layers of nesting within a module

Neither case seems to be ideal, nor does it feel like the direction the JavaScript community is taking. But more than that, there's a technical constraint which is that Babel's single-file-at-a-time model doesn't really support cross-file resolution.

The ability to export a single Button and then be able to reference Button.Props is nice and is a bit cleaner than having Button and ButtonProps as two exports but I don't think that gain is worth the overhead of making people understand what namespaces are and how they work. Having two exports is a lot more familiar to both library writers and consumers.

[8eecf0d2]

Thanks for the response @BPScott, appreciate your take on it.

If you're ever in Melbourne let me buy you a coffee! ☕️

[BPScott]

Two other thoughts that make me lean towards exports over namespaces: There's currently an issue where eslint reports the use of a namespace as a redeclaration linting error:

import React from 'react';

export declare namespace Button {
  export interface Props {
    x: string;
  }
}

// The below line has an eslint error saying `'Button' is already defined (no-redeclare)`
export function Button(props: Button.Props) {
  return <React.Fragment>{props.x}</React.Fragment>;
}

For us, changing to this method of exporting props would be a breaking change for us as currently we export ButtonProps for use by our consumers, obviously this doesn't apply to greenfield projects but that existing inertia makes this kind of thing tough to change and it'd require a solid quality of life improvement to make that change worth it, and I don't think the namespaces approach is worth effort to change.

For the sake of completeness there is a 3rd approach - React's types exposes ComponentProps, ComponentPropsWithRef and ComponentPropsWithoutRef types that lets you extract the props from a component. That feels really tempting, but it's a breaking change for us and I'm not sure how I feel about how discoverable this is for consumers. If you like the idea of not exposing the props as an export then this might be a good approach if you can clearly document it. that said, I don't think we ever explicitly say "for a Button component export there will be a corresponding ButtonProps export" in our docs and everything seems to have worked out fine so far.

// Button.tsx

import React from 'react';

interface Props {
  x: string;
}

export function Button(props: Props) {
  return <p>props.x</p>;
}

// Some file that consumes Button

import React from 'react';
import {Button} from './Button';

// this type is equal to the Props defined in button
type ButtonProps = React.ComponentPropsWithoutRef<typeof Button>;

[8eecf0d2]

I hadn't seen ComponentPropsWithoutRef before and while its a cute utility my usage of namespace would potentially be used to store miscellaneous component types and not just their Props:

export class Button extends React.Component<Button.Props, Button.State> {}

export namespace Button {
  export enum Type {
    NORMAL = "normal",
    DESTRUCTIVE = "destructive",
  }
  export interface Props {
    type: Button.Type;
  }
  export interface State {}
}

const jsx = (
  <Button type={Button.Type.DESTRUCTIVE} />
);

However I understand your reasoning and think I'll end up joining the {Button, ButtonProps} crowd soon, thanks again for the thorough review.

[tmlayton]

I’m in favor of this 👍

[amrocha]

The way we're doing it right now does feel a bit convoluted, and I agree with your changes, but I think we can go further. I wonder if we're not just creating these problems for ourselves by using index files for every component.

The issues you're pointing out seem especially bad because we're exporting everything twice. I think we can skip that step and just have a single index file for all components.

For example, if we did something like this:

// ./components/Button/Button.tsx
export Button;
export ButtonProps;

// ./components/index.ts
export {Button, ButtonProps} from './Button/Button.tsx';

we'd have something simpler than what we have right now, and just as functional.

[BPScott]

I wonder if we're not just creating these problems for ourselves by using index files for every component.

Killing index files would help avoid some indirection but they might be tougher to remove from a social perspective.

The shopify convention is that index files are the public API of a component. The strict-component boundaries linting rule helps enforce this by complaining if you try and do import {BLAH} from '../Component/Component';. We're can change that rule perhaps by adding an option to say we consider that file the index. I'm also leaning towards '../Component/Component' being pretty ugly, but modern es modules stuff is going to eventually push people away from folder name imports to using absolute paths so personal preference be damned in that case and ../Component/Component.js is probably better than ../Component/index.js.

Removing it would mean that the Component.ts acts as the public API, which is fine, but means we have to be stricter about what we export to ensure things don't inadvertently leak and become publicly available. This might make backwards compatibility annoying if we accidentally export things. The friction of having to add items to the index may help prevent that but that's may also be covered by "well we still have to export it in src/components/index` for something to be exposed publicly".

I'd be open to the idea of keeping the Component index files (e.g. components/Component/index.ts but them just being export * from './Component'. That's a little bit of boilerplate but it keeps us maintaining shopify convention of sibling components should import from the component index file and avoids the slightly ugly import {BLAH} from '../Component/Component'; style of imports.

---

Tangentially related to all this I've been playing with "What if we could use babel for our build" and currently the babel and rollup combo doesn't like it when we reexport Props (as babel acts on a per-file JS basis and it can't find exports of emit-less values). So killing off explict reexports of all that stuff and moving to exporting * would probably be pretty helpful in moving towards that goal.

We could say individual components will export *, but src/components/index.ts can do explicit exports (export {Avatar, AvatarProps} from './Avatar') so it acts as a gatekeeper for our public API. (components accidentally rexporting too much isn't a problem as long as those exports don't make it into the public API that can be imported from @shopify/polaris)

[amrocha]

The social reasons you mention don’t seem like strong reasons to me: if you don’t want something to be public, then don’t export it in the first place. I can be convinced otherwise if you showed me a good use case, like exporting something for tests that can't be tested otherwise.

The webpack reason is solid though.

This isn't an urgent problem or anything, just something I thought to question based on a few conversations I've had with the web foundations team. I'm ok with it if we keep it as is.

[BPScott]

Chatted with @amrocha, we're gonna punt on changing to use export * for component index files / killing the index files. It might still be a good idea, but it's not affecting us right now and I'm time-constrained and want to work on some other bits.

[BPScott]

Closed by #2058