Reformat exports #2058

BPScott · 2019-08-28T22:15:55Z

WHY are these changes introduced?

Most of an implementation for #1959. This is a big change so I'm going to split it into a few commits:

Convert sub-components
Convert top level components
Enable linting rules to alert on default exports

WHAT is this pull request doing?

Updates all components (and sub-components and sub-sub-components) so that:

They have a named export that matches the name of the component - e.g. in Button.ts the component will be exported as Button instead of a default export
They have a named export for props based on the component name - e.g. in Button.ts the props will be exported as ButtonProps instead of Props

This means that in component (/ subcomponent) indexes we don't need to do any renaming of exports when reexporting them, things keep the same name throughout the hierarchy.

There's still a few default exports used because of how we export things in withAppProvider and I think it is better to leave them be for the moment, The fix for them can be added in a separate PR where the changes won't get lost in all the other noise.

You can use the following script to search for items that have default or Props exports

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;

function containsAny(haystack, needles) {
  return haystack.some((name) => needles.includes(name));
}

// eslint-disable-next-line id-length
function caseInsensitiveCompare(a, b) {
  return a.localeCompare(b, undefined, {sensitivity: 'base'});
}

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 allExports = [];

    babelTraverse(ast, {
      ExportDefaultDeclaration() {
        allExports.push('default');
      },
      ExportSpecifier(path) {
        allExports.push(path.node.exported.name);
      },
    });

    allExports.sort(caseInsensitiveCompare);
    return {filePath, allExports};
  })
  .filter(({allExports}) => containsAny(allExports, ['default', 'Props']));

result.forEach((item) => {
  process.stdout.write(`${item.filePath}\n    ${item.allExports.join(', ')}\n`);
});

process.stdout.write(`\n\n${result.length} files found\n`);

amrocha

A few questions

src/components/Avatar/images/index.ts

src/components/Frame/components/CSSAnimation/CSSAnimation.tsx

src/components/Frame/components/Toast/index.ts

src/components/Frame/components/ToastManager/ToastManager.tsx

src/components/Page/components/Header/tests/Header.test.tsx

amrocha · 2019-08-28T22:58:01Z

src/components/ResourceList/components/FilterControl/components/FilterCreator/FilterCreator.tsx

@@ -207,4 +207,4 @@ class FilterCreator extends React.PureComponent<CombinedProps, State> {
  };
 }

-export default withAppProvider<Props>()(FilterCreator);
+export default withAppProvider<FilterCreatorProps>()(FilterCreator);


amrocha · 2019-08-28T22:58:06Z

src/components/ResourceList/components/FilterControl/components/FilterCreator/index.ts

-
-export {Props} from './FilterCreator';
-export default FilterCreator;
+export {default as FilterCreator, FilterCreatorProps} from './FilterCreator';


amrocha · 2019-08-28T22:59:23Z

...ResourceList/components/FilterControl/components/FilterValueSelector/FilterValueSelector.tsx

@@ -133,4 +133,4 @@ function buildOperatorOptions(operatorText?: string | Operator[]) {
  });
 }

-export default withAppProvider<Props>()(FilterValueSelector);
+export default withAppProvider<FilterValueSelectorProps>()(FilterValueSelector);


amrocha · 2019-08-28T22:59:30Z

src/components/ResourceList/components/FilterControl/components/FilterValueSelector/index.ts

-export {Props} from './FilterValueSelector';
-export default FilterValueSelector;
+export {
+  default as FilterValueSelector,


amrocha · 2019-08-28T23:08:52Z

src/components/TopBar/components/UserMenu/tests/UserMenu.test.tsx


 describe('<UserMenu />', () => {
-  const userMenuProps: Props = {
+  const userMenuProps = {


Why change this here?

If we don't need the typehint here, then we can avoid importing UserMenuProps entirely

oh, does TS infer the type?

yep, it'll complain if we miss a prop that needs to be passed in

amrocha · 2019-08-28T23:12:40Z

My comments make more sense in split diff view

amrocha · 2019-08-29T22:45:13Z

src/components/Navigation/index.ts

@@ -1,8 +1,8 @@
 export {Navigation, NavigationProps} from './Navigation';

 export {
-  ItemProps,
+  ItemProps as NavigationItemProps,


Fine for this PR, but let's revisit this, it might make sense to rename the entire component

amrocha · 2019-08-29T22:45:27Z

src/components/Navigation/index.ts

  SubNavigationItem,
  isNavigationItemActive,
-  MessageProps,
+  MessageProps as NavigationMessageProps,


Same as above

Tidy up a few bits of spacing along the way so consistently have a single line between export declarations

Add several exceptions for cases where we still use a default export because we have to wrap things in withAppProvider. Their number shall shrink over time as we migrate to use hooks for these components

Some stuff in web touches this private type. They're doing a naughty thing but lets keep the status quo for now and we can chide them later

BPScott force-pushed the reformat-exports branch from 40bf5df to 46be171 Compare August 28, 2019 22:18

BPScott requested a review from amrocha August 28, 2019 22:21

BPScott force-pushed the reformat-exports branch from 46be171 to e3de313 Compare August 28, 2019 22:28

amrocha suggested changes Aug 28, 2019

View reviewed changes

amrocha approved these changes Aug 29, 2019

View reviewed changes

amrocha reviewed Aug 30, 2019

View reviewed changes

BPScott added 6 commits August 29, 2019 17:07

Reformat subcomponents to export named values

6a6e825

Tidy up a few bits of spacing along the way so consistently have a single line between export declarations

Fix linting warning

6876ff1

Top level exports

27b5675

Add lint rule to warn on default exports

133e050

Add several exceptions for cases where we still use a default export because we have to wrap things in withAppProvider. Their number shall shrink over time as we migrate to use hooks for these components

A few more renames

b4eb1f7

Readd an export

42ed566

Some stuff in web touches this private type. They're doing a naughty thing but lets keep the status quo for now and we can chide them later

BPScott force-pushed the reformat-exports branch from 787d4d7 to 42ed566 Compare August 30, 2019 00:09

BPScott merged commit 44060b1 into master Aug 30, 2019

BPScott deleted the reformat-exports branch August 30, 2019 00:23

BPScott mentioned this pull request Aug 30, 2019

Proposal: Name components and Props once #1959

Closed

shanesmith mentioned this pull request Feb 26, 2020

[OptionList] Export OptionListProps #2777

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reformat exports #2058

Reformat exports #2058

BPScott commented Aug 28, 2019 •

edited

Loading

amrocha left a comment

amrocha Aug 28, 2019

amrocha Aug 28, 2019

amrocha Aug 28, 2019

amrocha Aug 28, 2019

amrocha Aug 28, 2019

BPScott Aug 28, 2019

amrocha Aug 28, 2019

BPScott Aug 29, 2019 •

edited

Loading

amrocha commented Aug 28, 2019

amrocha Aug 29, 2019

amrocha Aug 29, 2019

Reformat exports #2058

Reformat exports #2058

Conversation

BPScott commented Aug 28, 2019 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

amrocha left a 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

BPScott Aug 29, 2019 • edited Loading

Choose a reason for hiding this comment

amrocha commented Aug 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BPScott commented Aug 28, 2019 •

edited

Loading

BPScott Aug 29, 2019 •

edited

Loading