Skip to content

Conversation

@BPScott
Copy link
Member

@BPScott BPScott commented Aug 28, 2019

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

@BPScott BPScott requested a review from amrocha August 28, 2019 22:21
Copy link
Contributor

@amrocha amrocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions

}

export default withAppProvider<Props>()(FilterCreator);
export default withAppProvider<FilterCreatorProps>()(FilterCreator);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?


export {Props} from './FilterCreator';
export default FilterCreator;
export {default as FilterCreator, FilterCreatorProps} from './FilterCreator';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

}

export default withAppProvider<Props>()(FilterValueSelector);
export default withAppProvider<FilterValueSelectorProps>()(FilterValueSelector);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

export {Props} from './FilterValueSelector';
export default FilterValueSelector;
export {
default as FilterValueSelector,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?


describe('<UserMenu />', () => {
const userMenuProps: Props = {
const userMenuProps = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, does TS infer the type?

Copy link
Member Author

@BPScott BPScott Aug 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@amrocha
Copy link
Contributor

amrocha commented Aug 28, 2019

My comments make more sense in split diff view


export {
ItemProps,
ItemProps as NavigationItemProps,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

SubNavigationItem,
isNavigationItemActive,
MessageProps,
MessageProps as NavigationMessageProps,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 BPScott merged commit 44060b1 into master Aug 30, 2019
@BPScott BPScott deleted the reformat-exports branch August 30, 2019 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants