Polaris-react: Post v4 cleanup work

Created on 20 Aug 2019 · 8Comments · Source: Shopify/polaris-react

This is a meta issue to collate what I'd like to do as tidy-up work in a post-v4 world (in v4.1.0, v4.2.0 and beyond).

I'd like for us to do three things, in vague order of priority. Much of this conversion work has several quick wins for components where a migration to the new style is easy, we should prioritize that easy work, and punt on the fewer harder migrations. That way our first timebox of work shall feel like good progress and make future planning easier (as there are fewer items to deal with overall)

DONE! ~Convert all examples to use hooks instead of classes.~

Shopify is rallying around hooks as the new best practice - we should help ensure people follow that by giving up to date examples.

Searching for "extends React." in README.md files should return no results.

@AndrewMusgrave made a pretty big start on this in https://github.com/Shopify/polaris-react/compare/hookify-examples but I'm not sure how polished that branch is. It might be worth splitting off chunks of that branch into separate PRs (5 or so README files at a time depending on the ease of the comparison) and reviewing them individually so the changes are more digestible.

Convert existing code to use hooks instead of classes

Hooks are the way forward and we should leverage them. We should prioitize components that use withAppProvider (as we want to remove that HoC), and small components where making the change is easy.

@AndrewMusgrave made a start on this in https://github.com/Shopify/polaris-react/compare/fast-hookifies but I am mot sure how polished that branch is. As above, splitting this out into multiple PRs for ease of review would be useful

Migrate our tests from `enzyme` to use `@shopify/react-testing`

This is a bunch of work with not much value behind it (aside from enzyme being often low-key annoying), this can probably be deferred and deferred until the end of time.

One the up side @vsumner has wrote a codemod in web that will bulk convert a codebase so that'd be worth investigating.

Source

BPScott

Most helpful comment

can I migrate Frame/components/Loading/Loading.tsx and Frame/components/ Toast/Toast.tsx?

sijad on 2 Oct 2019

❤1 👍1

All 8 comments

Audits for hooks conversion

To help identify where we can focus for quick wins, here's an audit of the difficulty of migrating components (half done, I still need to finish off several files).

Files using withAppProvider are a priority, then easy other class-based components

Difficulty is a 1 to 5 scale, with 1 being easiest, and 5 being hardest.

1 is a component with a state or two that need to be convered to use useState
2 is a component with a few states and a few callback handlers that need wrapping in useCallback
3 is a component with lifecycle hooks (didMount etc) that will need writing a useEffect
4 is somewhere between 3 and 5
5 is a biiiiiiiig component that has lots of hooks and lifecycle effects

Blasting out 5 or so 1 or 2 level complexity components in a single PR is fine as changes should be reasonably straightforward to understand (and pretty minimal if you supress whitespace in the diff), but beyond that review gets harder so one or two components per PR.

DONE! ~Files using withAppProvider (~32~ 0), to hookify~

withAppProvider has been removed as of v5.

Files requiring weird casting to keep TS happy in web (1)

Confirm that web works after converting to hooks

| Filename | Difficulty |
| --------------------------- | ---------- |
| Collapsible/Collapsible.tsx | 3.5 |

Files containing ignored eslint warnings around state management (~7~ 3)

Found by searching for eslint-disable-next-line react/

| Filename | Difficulty | Already mentioned |
| --------------------------------------------- | ---------- | ----------------- |
| AppProvider/AppProvider.tsx | 3 | - |
| Collapsible/Collapsible.tsx | 3.5 | Y |
| ResourceList/ResourceList.tsx | 5 | Y |

Files using "globalIdGeneratorFactory" when using useUniqueId() would be a preferable (~3~ 1)

Found by searching for "globalIdGeneratorFactory"

| Filename | Difficulty | Already mentioned |
| --------------------------------------------- | ---------- | ----------------- |
| ResourceItem/ResourceItem.tsx | 5 | Y

All components that extend from React.Component or React.PureComponent (~82~ 33)

Found by searching for extends Component and extends PureComponent

| Filename | Difficulty | Already mentioned |
| -------------------------------------------------------------------------------------------- | -------------- | ----------------- |
| AppProvider/AppProvider.tsx | 3 | Y |
| Breadcrumbs/Breadcrumbs.tsx | | @AndrewMusgrave |
| Collapsible/Collapsible.tsx | 3.5 | Y |
| ColorPicker/ColorPicker.tsx | | - |
| ColorPicker/components/ AlphaPicker/AlphaPicker.tsx | | - |
| ColorPicker/components/ HuePicker/HuePicker.tsx | | - |
| ColorPicker/components/ Slidable/Slidable.tsx | | - |
| Connected/components/ Item/Item.tsx | | - |
| DataTable/DataTable.tsx | 5 | Y |
| EventListener/EventListener.tsx | | @AndrewMusgrave |
| Filters/components/ ConnectedFilterControl/components/ Item/Item.tsx | | @AndrewMusgrave |
| Filters/components/ ConnectedFilterControl/ConnectedFilterControl.tsx | | @AndrewMusgrave |
| Filters/Filters.tsx | 5 | Y |
| Frame/components/ Loading/Loading.tsx | | @sijad |
| Frame/Frame.tsx | | - |
| List/List.tsx | | @AndrewMusgrave |
| Modal/Modal.tsx | 4 | Y |
| Navigation/components/ Section/Section.tsx | | @AndrewMusgrave |
| Navigation/Navigation.tsx | | - | @AndrewMusgrave
| Page/Page.tsx | 3.5 | Y |
| Popover/components/ PopoverOverlay/PopoverOverlay.tsx | | - |
| PositionedOverlay/PositionedOverlay.tsx | | - |
| RangeSlider/components/ DualThumb/DualThumb.tsx | | - |
| ResourceItem/ResourceItem.tsx | 5 | - |
| ResourceList/components/ BulkActions/BulkActions.tsx | 5 | Y |
| ResourceList/ResourceList.tsx | 5 | Y |
| ResourcePicker/ResourcePicker.tsx | 3 (deprecated) | Y |
| Scrollable/Scrollable.tsx | | @AndrewMusgrave |
| Sticky/Sticky.tsx | 3 | Y |
| Tabs/components/ Item/Item.tsx | | @AndrewMusgrave |
| Tabs/components/ List/List.tsx | | @AndrewMusgrave |
| Tabs/Tabs.tsx | 3.5 | Y |
| Tooltip/components/ TooltipOverlay/TooltipOverlay.tsx | | @AndrewMusgrave |