Fix consistency issue in prop types

[amrocha]

WHY are these changes introduced?

tl;dr: The Props Explorer type parsing script makes assumptions about the format of the data that were never documented, and this update broke those assumptions. This fixes that. Longer version follows, read on if you're interested in how the Props Explorer works.

So, this repository interacts pretty closely with https://github.com/Shopify/polaris-styleguide . The styleguide has a feature called the Props Explorer, which allows users to search through any given component's props in a tree-like manner, right from the styleguide, and check out their types, default values, names, etc.

To be able to do this, we implemented the following pipeline on the styleguide:

1. Download the polaris-react package as a dependency
2. Run typedoc on the types, to generate a JSON file representation of these types
3. Execute a script that reads this JSON file and extracts the data that we need per component
4. Run this script during our server setup and put the results in our GraphQL API
5. Consume GraphQL API to render the Props Explorer

When we updated to @shopify/polaris's latest version step 3 stopped working. Part of the problem is that it was failing silently, and thus hard to debug. We don't have a lot of documentation or tests written for the Props Explorer, so a lot of this exploration was manual guesswork.

typedoc parses the types generated by TS, and spits out an object. This object contains all the types and functions declared in the polaris-react codebase. The crux of the problem is that typedoc gives you limited information. It will tell you the signature and type of the properties of any function, but won't tell you what the name of the interface that the function implements is.

To deal with this, the original writer of this code made an assumption that all component files would declare a Props interface, export it, and implement it as its props. This means that if you:

1. Import an interface and implement it or;
2. Declare an interface named anything other than Props;

The script breaks. Going forward we should look into ways of enforcing this consistently through a linting rule.

WHAT is this pull request doing?

Renaming and moving a few type interfaces to match the format expected by the Props Explorer.

How to 🎩

Build for the styleguide and check if the props explorer shows up

[kaelig]

Here's the style guide's Percy report for this branch: https://percy.io/Shopify/polaris-styleguide/builds/1225951

It found 37 unreviewed changes, somebody would need to go through them.

Icons

I found a regression with the icon component, that doesn't show a list of all icons anymore (maybe that's something that needs to be fixed on the style guide itself?):

The same happens in the banner component props explorer:

Action type

The props of Action type aren't clickable anymore (seen in Card, Empty state…):

Before

After

Card

There's a weird bug with the Card dropdown menu (see last example in https://polaris-react-pr-727.herokuapp.com/card) in the Percy report that I can't reproduce:

[amrocha]

A lot of those changes don't look like they're related to this PR specifically, but to the 3.2 update. Went through it and didn't see anything that would be specifically related to this PR(the Props Explorer would have changed).

[BPScott]

At very real risk of a "Ben: go way you're on holiday" (to which I reply "I am bad at disconnecting, thank you for trying not to enable this bad behaviour <3"):

Reverting these changes takes us a step away from being able to enable the "import/no-cycle" eslint rule, which I want to enable so that we can confidently say sewing-kit can stop stop rebuilding polaris with a custom env because our default output when compiled for production doesn't work. Strictly speaking I think circular type imports aren't a problem for SK - only component imports - but the no-cycle rule does not distinguish between them.

I'm good with merging this in as an interim measure to get the styleguide up and running with v3.2, but I think the long-term fix is making our type doc tooling in polaris-styleguide be smart enough to handle the valid typescript that gets thrown at it, instead of limiting how we write code in polaris-react - I had no idea this would break things when I refactored and I don't think other people should need to know that there is a limitation either.

[amrocha]

Hey Ben. The first time I worded my comment playfully that you're not supposed to be here but it seems you didn't take it seriously.

You're a senior member of the team, who might become a lead in the near future. This kind of behaviour is unacceptable. Think about the example you're setting for everyone else. About the impact you're having on the culture of the team. Not only the team but also the rest of Shopify. It's not ok to acknowledge that you're doing something wrong and keep doing it.

This is not an emergency. You're not the only person with context on this. You weren't even pinged on this!

Trust that we're taking good care of the codebases when you leave. Of course this is a less than ideal solution. Of course we're looking into fixing this on the script side, so that we can get to a point where all valid TS is accepted. Trust us. We're not stupid. We can get around to it and discuss it with you after you come back from vacation.

[amrocha]

Built this for the styleguide, no issues found, everything working as expected

[danrosenthal]

Can you add a changelog entry? I'm going to give this a check in web

[amrocha]

@danrosenthal just did!

[BPScott]

tophatted in polaris-styleguide.

I got some type errors when tophatting in web, but they seemed to be due to the polaris-tokens bump rather than anything in this PR

[danrosenthal]

I got some type errors when tophatting in web, but they seemed to be due to the polaris-tokens bump rather than anything in this PR

@BPScott can you share the errors you got here?

[BPScott]

[dealt with in Slack]