Use export * in component indexes and subcomponent reexports #2625

BPScott · 2020-01-16T23:28:42Z

WHY are these changes introduced?

Updating component indexes to use export *. Fixes #2619

277 changed files. Really Ben?

Yeah, sorry. It probably best to review this PR by commit by commit.

WHAT is this pull request doing?

This PR does a few things.

First, a trio of commits that updates all the component (and subcomponent) files (e.g. src/components/ActionMenu/ActionMenu.tsx and src/components/Frame/components/CSSAnimation.tsx) to ensure that the component file exports only the same as what the current index exports.

Second, now that all component files and their indexes (e.g. src/components/ActionMenu/ActionMenu.tsx and src/components/ActionMenu/index.ts) export the same things, update all component indexes to use export *.

Thirdly, update all subcomponent folder indexes (e.g. src/components/Frame/components/index.ts) to reexport to use export *

How to 🎩

Ensure type-check and tests pass.
Note that the contents of the utilities folder and src/components/index.ts have not been touched so the public exports accessed by importing @shopify/polaris are untouched, with the exception of adding the SheetProps, which seem useful publicly.

Once https://github.com/Shopify/web/pull/22624 has been merged into web to stop them reaching into our internal types, then ensure that web passes a type-check with these changes:

[in polaris] yarn run build-consumer web
[in web] yarn run type-check

github-actions · 2020-01-16T23:30:59Z

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified	30
Files potentially affected	63

Details

All files potentially affected (total: 63)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/AccountConnection/index.ts (total: 0)

Files potentially affected (total: 0)

🧩 src/components/ActionList/components/Item/index.ts (total: 19)

Files potentially affected (total: 19)

🧩 src/components/ActionList/components/Section/index.ts (total: 18)

Files potentially affected (total: 18)

🧩 src/components/ActionList/components/index.ts (total: 18)

Files potentially affected (total: 18)

🧩 src/components/ActionList/index.ts (total: 17)

Files potentially affected (total: 17)

🧩 src/components/ActionMenu/ActionMenu.tsx (total: 2)

Files potentially affected (total: 2)

🧩 src/components/ActionMenu/components/MenuAction/index.ts (total: 4)

Files potentially affected (total: 4)

🧩 src/components/ActionMenu/components/MenuGroup/index.ts (total: 3)

Files potentially affected (total: 3)

🧩 src/components/ActionMenu/components/RollupActions/index.ts (total: 3)

Files potentially affected (total: 3)

🧩 src/components/ActionMenu/components/index.ts (total: 3)

Files potentially affected (total: 3)

🧩 src/components/ActionMenu/index.ts (total: 2)

Files potentially affected (total: 2)

🧩 src/components/ActionMenu/tests/ActionMenu.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/AfterInitialMount/index.ts (total: 0)

Files potentially affected (total: 0)

🧩 src/components/AppProvider/index.ts (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Autocomplete/components/ComboBox/ComboBox.tsx (total: 1)

Files potentially affected (total: 1)

/src/components/Autocomplete/Autocomplete.tsx

🧩 src/components/Autocomplete/components/ComboBox/index.ts (total: 1)

Files potentially affected (total: 1)

/src/components/Autocomplete/Autocomplete.tsx

🧩 src/components/Autocomplete/components/TextField/index.ts (total: 1)

Files potentially affected (total: 1)

/src/components/Autocomplete/Autocomplete.tsx

🧩 src/components/Autocomplete/components/index.ts (total: 1)

Files potentially affected (total: 1)

/src/components/Autocomplete/Autocomplete.tsx

🧩 src/components/Autocomplete/index.ts (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Avatar/Avatar.tsx (total: 12)

Files potentially affected (total: 12)

🧩 src/components/Avatar/index.ts (total: 12)

Files potentially affected (total: 12)

🧩 src/components/Avatar/tests/Avatar.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Backdrop/index.ts (total: 7)

Files potentially affected (total: 7)

🧩 src/components/Badge/Badge.tsx (total: 26)

Files potentially affected (total: 26)

🧩 src/components/Badge/index.ts (total: 26)

Files potentially affected (total: 26)

🧩 src/components/Badge/tests/Badge.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Banner/index.ts (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Breadcrumbs/index.ts (total: 2)

Files potentially affected (total: 2)

🧩 src/components/Button/Button.tsx (total: 52)

Files potentially affected (total: 52)

This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

BPScott · 2020-01-16T23:31:28Z

src/components/Avatar/Avatar.tsx

@@ -9,15 +9,15 @@ import {Image} from '../Image';
 import styles from './Avatar.scss';
 import * as avatars from './images';

-export type Size = 'small' | 'medium' | 'large';
+type Size = 'small' | 'medium' | 'large';


This sort of thing crops up a lot. Using this particular case as an example, Size was exported here but was never reexported by src/Avatar/index.ts and was never used in any other file, so there is no value in exporting it at all.

BPScott · 2020-01-16T23:34:33Z

src/components/Avatar/tests/Avatar.test.tsx

@@ -116,10 +115,6 @@ describe('<Avatar />', () => {
  });

  describe('styleClass', () => {
-    it('defaults to five styles', () => {


This is testing an internal implementation detail, I didn't want to expose STYLE_CLASSES so it was easiest to remove this test instead of moving files around to avoid exposing STYLE_CLASSES

BPScott · 2020-01-16T23:35:22Z

src/components/Button/tests/Button.test.tsx

@@ -168,7 +168,7 @@ describe('<Button />', () => {

    it('does not render the markup for the icon if none is provided', () => {
      const button = mountWithAppProvider(<Button />);
-      expect(button.find(IconWrapper).exists()).toBe(false);


IconWrapper shouldn't be exposed outside of Button.

BPScott · 2020-01-16T23:36:42Z

src/components/Checkbox/Checkbox.tsx

@@ -11,7 +11,7 @@ import {Error, Key, CheckboxHandles} from '../../types';

 import styles from './Checkbox.scss';

-export interface BaseProps {
+export interface CheckboxProps {


This crops up in a few files - a BaseProps interface and the component's props that extend from that without adding anything news - so remove BaseProps.

BPScott · 2020-01-16T23:37:32Z

src/components/ChoiceList/tests/ChoiceList.test.tsx


 describe('<ChoiceList />', () => {
-  let choices: ChoiceDescriptor[];
+  let choices: ChoiceListProps['choices'];


Where possible we should look at the values that live on the Props interface instead of exposing the interfaces the Props use

BPScott · 2020-01-16T23:43:12Z

src/components/Popover/Popover.tsx

@@ -12,10 +12,10 @@ import {PreferredPosition, PreferredAlignment} from '../PositionedOverlay';
 import {Portal} from '../Portal';
 import {portal} from '../shared';
 import {useUniqueId} from '../../utilities/unique-id';
-import {CloseSource, Pane, PopoverOverlay, Section} from './components';
+import {PopoverCloseSource, Pane, PopoverOverlay, Section} from './components';


Name this import/export correctly where it is initially defined, avoids having to rename it in the Popover/index.ts file

BPScott · 2020-01-16T23:45:38Z

src/components/Sheet/tests/Sheet.test.tsx

-      BOTTOM_CLASS_NAMES,
-    );
+    expect(sheet.find(CSSTransition).props().classNames).toStrictEqual({
+      enter: 'Bottom enterBottom',


This is a bit meh, but I think it's a bit better than exposing the BOTTOM_CLASS_NAMES RIGHT_CLASS_NAMES or extracting them out into a file that used by both Sheet and Sheet.test.tsx

BPScott · 2020-01-16T23:46:11Z

src/components/Spinner/tests/Spinner.test.tsx

@@ -75,12 +75,10 @@ describe('<Spinner />', () => {
    it('a large spinner with an unavailable color warns in development', () => {
      process.env.NODE_ENV = 'development';

-      const color = 'black' as Color;


That's not even a valid Color shrug

BPScott · 2020-01-16T23:47:39Z

src/components/Collapsible/Collapsible.tsx

@@ -34,7 +34,7 @@ interface State {

 const ParentCollapsibleExpandingContext = createContext(false);

-class Collapsible extends React.Component<CollapsibleProps, State> {
+class CollapsibleInner extends React.Component<CollapsibleProps, State> {


Kill off cases where we export a default value - now things that use AppProvider are exported with a named component, which means that the component index file doesn't need to rename the default export to a named export

BPScott · 2020-01-16T23:50:00Z

src/components/PositionedOverlay/tests/PositionedOverlay.test.tsx

@@ -124,99 +120,6 @@ describe('<PositionedOverlay />', () => {
    });
  });

-  describe('intersectionWithViewport', () => {


This is testing internal details, presumably because it's hard to to test this using just the component.

Move this function to math.ts and do the tests in math.test.ts to avoid exporting intersectionWithViewport from PositionedOverlay.tsx

amrocha

We went over this together, looks good once the problems in web have been fixed

BPScott requested a review from amrocha January 16, 2020 23:28

BPScott commented Jan 16, 2020

View reviewed changes

BPScott force-pushed the export-star-components branch 2 times, most recently from 4e4b355 to cb56bfa Compare January 17, 2020 19:47

amrocha approved these changes Jan 17, 2020

View reviewed changes

BPScott added 6 commits January 17, 2020 14:07

Fixup component files to they don't export more than their index

bdb8432

Fixup PositionedOverlay so it doesn't export more than its index

f25e7d0

Convert components using default exports to use named export

2fdb183

Update component indexes to use export *

dd74290

Update component subcomponent indexes to use export *

41d3267

Add changelog entry

f2ec124

BPScott force-pushed the export-star-components branch from cb56bfa to f2ec124 Compare January 17, 2020 22:08

BPScott merged commit a487f75 into master Jan 17, 2020

BPScott deleted the export-star-components branch January 17, 2020 22:18

This was referenced Jan 18, 2020

Type tidying #2631

Merged

Use export * in utility indexes too #2636

Merged

danrosenthal restored the export-star-components branch January 23, 2020 15:17

dleroux temporarily deployed to production January 27, 2020 13:38 Inactive

shanesmith mentioned this pull request Feb 26, 2020

[OptionList] Export OptionListProps #2777

Merged

danrosenthal deleted the export-star-components branch February 27, 2020 19:57

Use export * in component indexes and subcomponent reexports #2625

Use export * in component indexes and subcomponent reexports #2625

Conversation

BPScott commented Jan 16, 2020 • edited Loading

WHY are these changes introduced?

277 changed files. Really Ben?

WHAT is this pull request doing?

How to 🎩

github-actions bot commented Jan 16, 2020 • edited Loading

Details

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 19)

Files potentially affected (total: 18)

Files potentially affected (total: 18)

Files potentially affected (total: 17)

Files potentially affected (total: 2)

Files potentially affected (total: 4)

Files potentially affected (total: 3)

Files potentially affected (total: 3)

Files potentially affected (total: 3)

Files potentially affected (total: 2)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 1)

Files potentially affected (total: 1)

Files potentially affected (total: 1)

Files potentially affected (total: 1)

Files potentially affected (total: 0)

Files potentially affected (total: 12)

Files potentially affected (total: 12)

Files potentially affected (total: 0)

Files potentially affected (total: 7)

Files potentially affected (total: 26)

Files potentially affected (total: 26)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 2)

Files potentially affected (total: 52)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amrocha left a comment

Choose a reason for hiding this comment

BPScott commented Jan 16, 2020 •

edited

Loading

github-actions bot commented Jan 16, 2020 •

edited

Loading