Skip to content

Conversation

BPScott
Copy link
Member

@BPScott BPScott commented Jan 8, 2019

WHY are these changes introduced?

Typedoc (which we use for generating prop tables on the styleguide) has
a bug in it where it doesn't correctly resolve types that are imported
from a different file (or we're using it wrong).
Instead of looking at imports it seems to look at a global bucket of
names. In this case there was a clash between Option (as living in Select)
and Option (as living in OptionList/components).

This resulted it it thinking that the type for ResourceList -> sortOptions was src/components/OptionList/components/Option/Option.tsx's default export instead of the type in src/components/Select/Select.tsx

WHAT is this pull request doing?

Work around this by renaming Select's Option to "SelectOption".

This allows our type documentation generation in the styleguide to work
correctly.

Strictly speaking the change to SelectGroup isn't needed but i think
"SelectOption and SelectGroup" sounds nicer than "SelectOption and
Group".

How to 🎩

  • In polaris-styleguide checkout this PR: https://github.com/Shopify/polaris-styleguide/pull/2488
  • In polaris-styleguide run yarn run typedoc and swear loudly because it fails.
  • In polaris-react run yarn build-consumer polaris-styleguide
  • In polaris-styleguide run yarn run typedoc again and see that it runs without errors. Cheer / run your temples /swear loudly again.
  • In polaris-styleguide run yarn dev and see that the prop table on the ResourceList page correctly states that sortOptions has the typing string | StrictOption[] just like in production

@BPScott BPScott temporarily deployed to polaris-react-pr-830 January 8, 2019 01:45 Inactive
@BPScott
Copy link
Member Author

BPScott commented Jan 8, 2019

From @AndrewMusgrave on Slack "Don’t ask me why but this seems to be the culprit (719ca66). I can’t for the life of me see why though"

I think that file changed the order in which typedoc discovered files which resulted in a different Option being set as the "global Option" (for want of a better phrase).

Props as SelectProps,
Option as SelectOption,
Group as SelectGroup,
SelectOption,
Copy link
Member Author

Choose a reason for hiding this comment

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

As we're not changing the export names in src/components/index.ts this is not a breaking change to our public API

import Spinner from '../Spinner';
import {withAppProvider, WithAppProviderProps} from '../AppProvider';
import Select, {Option} from '../Select';
import Select, {SelectOption} from '../Select';
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried changing just the import statement to import Select, {Option as SelectOption} from '../Select'; but that did not work.

yarn run typedoc compiled correctly but when viewing the types it just said SelectOption[] instead of exposing what that intersection type referred to (i.e. string | StrictOption[]) like on production.

@danrosenthal danrosenthal force-pushed the workaround-typedoc-bugs branch from 235d100 to 0f7ed01 Compare January 8, 2019 14:16
@BPScott BPScott temporarily deployed to polaris-react-pr-830 January 8, 2019 14:16 Inactive
Copy link

@danrosenthal danrosenthal left a comment

Choose a reason for hiding this comment

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

🎩

Typedoc (which we use for generating prop tables on the styleguide) has
a bug in it where it doesn't correctly resolve types that are imported
from a different file (or we're using it wrong).
Instead of looking at imports it seems to look at a global bucket of
names. In this case there was a clash between Option (as living in Select)
and Option (as living in OptionList/components). Work around this by
renaming Select's Option to "SelectOption".

This allows our type docmentation generation in the styleguide to work
correctly.

Strictly speaking the change to SelectGroup isn't needed but i think
"SelectOption and SelectGroup" sounds nicer than "SelectOption and
Group".
Copy link
Member

@AndrewMusgrave AndrewMusgrave left a comment

Choose a reason for hiding this comment

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

I think that file changed the order in which typedoc discovered files which resulted in a different Option being set as the "global Option" (for want of a better phrase).

Makes sense 🎩 💯

@danrosenthal danrosenthal merged commit 3539c37 into master Jan 8, 2019
@danrosenthal danrosenthal deleted the workaround-typedoc-bugs branch January 8, 2019 15:11
@danrosenthal danrosenthal temporarily deployed to production January 8, 2019 15:33 Inactive
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.

3 participants