Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Post v4 cleanup work #1995

Closed
BPScott opened this issue Aug 20, 2019 · 14 comments
Closed

Post v4 cleanup work #1995

BPScott opened this issue Aug 20, 2019 · 14 comments

Comments

@BPScott
Copy link
Member

BPScott commented Aug 20, 2019

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: #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.

@BPScott
Copy link
Member Author

BPScott commented Aug 20, 2019

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

@sijad
Copy link
Contributor

sijad commented Sep 7, 2019

can I migrate Tabs/components/Tab/Tab.tsx?

@chloerice
Copy link
Member

can I migrate Tabs/components/Tab/Tab.tsx?

Absolutely!! I've updated the withAppProvider table to assign you 😺

@sijad sijad mentioned this issue Sep 11, 2019
6 tasks
@BPScott
Copy link
Member Author

BPScott commented Sep 11, 2019

Updated the tables to remove the completed items

@sijad
Copy link
Contributor

sijad commented Oct 2, 2019

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

@chloerice
Copy link
Member

chloerice commented Oct 2, 2019

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

Hey @sijad, go for it!! I've updated the table 👍

@Tom-Bonnike
Copy link
Contributor

Taking care of the Banner in #3199.

@alex-page
Copy link
Member

Taking care of Collapsible #3606

@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

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.

@BPScott
Copy link
Member Author

BPScott commented Apr 10, 2023

Migration to functional components is still worthwhile IMO.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

@alex-page
Copy link
Member

@BPScott I was going to get around to this 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants