Shopify
Post v4 cleanup work
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
Done ~Migrate our tests from enzyme to use @shopify/react-testing~
Dedicated issue: https://github.com/Shopify/polaris-react/issues/4274
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.
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.
All components that extend from React.Component or React.PureComponent (~82~ 20)
Found by searching for extends Component and extends PureComponent
| Filename | Difficulty | Notes |
|---|---|---|
| AppProvider/AppProvider.tsx | 3 | Has react lint warnings mentioning 'eslint-disable-next-line react/' |
| BulkActions/BulkActions.tsx | ||
| ColorPicker/ColorPicker.tsx | ||
| ColorPicker/components/ AlphaPicker/AlphaPicker.tsx | ||
| ColorPicker/components/ HuePicker/HuePicker.tsx | ||
| ColorPicker/components/ Slidable/Slidable.tsx | ||
| DataTable/DataTable.tsx | 5 | |
| DropZone/DropZone.tsx | ||
| EventListener/EventListener.tsx | ||
| Filters/components/ ConnectedFilterControl/ConnectedFilterControl.tsx | ||
| Filters/Filters.tsx | 5 | |
| Frame/Frame.tsx | ||
| Popover/components/ PopoverOverlay/PopoverOverlay.tsx | ||
| PositionedOverlay/PositionedOverlay.tsx | ||
| RangeSlider/components/ DualThumb/DualThumb.tsx | ||
| ResourceItem/ResourceItem.tsx | 5 | Uses "globalIdGeneratorFactory" when using useUniqueId() would be a preferable |
| Scrollable/Scrollable.tsx | ||
| Sticky/Sticky.tsx | 3 | |
| Tabs/components/ Item/Item.tsx | ||
| Tabs/Tabs.tsx | 3.5 |
can I migrate
Tabs/components/Tab/Tab.tsx?
Absolutely!! I've updated the withAppProvider table to assign you 😺
can I migrate Frame/components/Loading/Loading.tsx and Frame/components/ Toast/Toast.tsx?
can I migrate
Frame/components/Loading/Loading.tsxandFrame/components/ Toast/Toast.tsx?
Hey @sijad, go for it!! I've updated the table 👍
Taking care of the Banner in https://github.com/Shopify/polaris-react/pull/3199.
Taking care of Collapsible https://github.com/Shopify/polaris-react/pull/3606
This issue seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. → If there's no activity within a week, then a bot will automatically close this. Thanks for helping to improve Shopify's design system and dev experience.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hi! We noticed there hasn’t been activity on this issue in a while. After 30 days, it will close automatically.
If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed.
Hi! We noticed there hasn’t been activity on this issue in a while. After 30 days, it will close automatically.
If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed.
Hi! We noticed there hasn’t been activity on this issue in a while. After 30 days, it will close automatically.
If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed.