Proposal: Use export * in component indexes

[BPScott]

A bit of a follow on from #1959

I've been looking at the feasibility of updating our build. Currently we do a complicated dance where we compile using typescript, then transpile that using babel. I've been investigating if instead we could just have babel read our TS files directly, which ends up being a fair bit simpler, however we need to modify our code slightly so Babel can handle type reexports correctly.

We've had this problem for a while - our Storybook build also uses babel to transform our TS files - however we've been able to ignore/suppress the generated warnings till now.

The issue is that we have errors when we reexport types. e.g.

// src/components/ActionList/index.ts
export {ActionList, ActionListProps} from './ActionList';

When compiling with tsc it looks over the whole project and it knows that ActionListProps is a type and thus should be removed from the compiled output. However babel only looks at a single file at a time so it doesn't know if ActionListProps is a type or value, so it assumes it is a value and leaves the export in. This however causes an issue in bundlers because when webpack/rollup processes this file it tries to find ActionListProps and then fails because it doesn't exist (as it is a type not a value). In webpack this results in those export ActionListProps was not found in... warnings, and is a hard error in rollup. Thus we need a way to stop these errors.

I would like to propose changing how we export in component indexes, so that we can give babel the information it needs to properly remove type imports.

TypeScript 3.8 will introduce type-only imports/exports (final release expected in mid February), which is new syntax that will give babel the information to remove the type exports. So we could rewrite the ActionList index as:

// src/components/ActionList/index.ts
export {ActionList} from './ActionList';
export type {ActionListProps} from './ActionList';

However I feel this repetition is a somewhat annoying. Instead I think we should use star exports, to export everything from the ActionList.ts file. This sidesteps the issue entirely as bundlers will take whatever is there instead of us having to repeat the names of components and their props:

// src/components/ActionList/index.ts
export * from './ActionList';

This feels safe to me as the majority of the time components only export themselves and their props. There may be some cases where we would want to tweak the file layout slightly to ensure exports that are only used within a component are not exposed outside the component but those cases are in the small minority.

---

This will fix the majority of webpack/rollup's complaints today, however there are two other cases that the above does not address:

The rexporting of all components in src/components/index.ts should move to use the export type notation and make two export calls - this duplication is a little meh but it allows us to strongly control what gets exported from library instead of a blanket export - this helps protect us against accidentally exposing values that should be internal.

The reexporting of types in our utilities e.g. export {Theme, ThemeConfig, CustomPropertiesLike, ColorScheme} from './types'; in src/utilities/theme/index.ts. In these cases we should also use the export type notation too as there would be no values in that type file, so no repeated reexports.

---

Does this seem like a reasonable proposal? Do you have any concerns?
If not I'll spin up a PR to make the changes to component indexes this/next week.

[ghost]

👋 Thanks for opening your first issue. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

[BPScott]

Pinging @chloerice, @AndrewMusgrave, @danrosenthal, @tmlayton, @alex-page, @kyledurand and @amrocha to get their thoughts on this.

[chloerice]

This is how I export in TS side projects. I'm by no means someone to advise you in the area of builds, but what you outlined makes sense to me.

[danrosenthal]

Thanks for spending time during our pairing to walk me through this. I'm in favor of the proposed change

[AndrewMusgrave]

Super excited about the type only imports 😍 This all makes sense to me. As long as we're following the export {}; export type {} syntax in src/components/index.ts there is no harm in exporting everything, and may even cut down on files changed in some pull requests since we won't have to play the export dance 😛 Go for it 💯

[amrocha]

I like the general export *; syntax, but is there a way to make sure all exports are done this way with a linting rule?

If yes then I'm all for it

[BPScott]

I like the general export *; syntax, but is there a way to make sure all exports are done this way with a linting rule?

No linting rule from what I can see in eslint-plugin-import, but we can enable isolatedModules: true in our tsconfig which will shout at us and fail the build if it comes across reexports of types that don't use the export type notation (and thus will cause webpack/rollup to complain).

[kyledurand]

This makes a bunch of sense to me 👍

[BPScott]

One concept I missed the first time around:

What should we do with subcomponent indexes (e.g. src/components/ActionList/components/index.ts). We want to avoid doing export * in the main src/components/index.ts to avoid accidentally exporting anything public, but the subcomponent indexes are slightly different - there's no risk of public leakage. I'm thinking having those use export * is fine too as we don't risk exposing anything beyond the top level component boundary.

[tmlayton]

+1 doing * exports from both component and subcomponent indexes