Skip to content

Conversation

AndrewMusgrave
Copy link
Member

@AndrewMusgrave AndrewMusgrave commented Dec 21, 2018

WHY are these changes introduced?

Improved HoC usage

  • withSticky
  • withAppProvider
  • withContext
  • withRef

WHAT is this pull request doing?

Letting typescript infer props rather than passing them as generics

Old
export default withAppProvider<Props>()(Component)
New
export default withAppProvider()(Component)

When we compose HoC we'll still have to pass props but only to compose

Old
export default compose<Props>(
  withContext<Props, DropZoneContext>(Consumer),
  withAppProvider<Props>(),
  withRef<Props>(),
)(FileUpload);
New
export default compose<Props>(
  withContext<DropZoneContext>(Consumer),
  withAppProvider(),
  withRef(),
)(FileUpload);

How to 🎩

This is a pretty big change so we'll want to test the components making use of different HoC.
Here's small list of some components you'll want to test making use of some HoC's

withSticky

  • Scrollable

withAppProvider

  • Button
  • DatePicker
  • OptionList
  • most components 😄

withContext

  • DropZone.FileUpload
  • ResourceList.FilterControl
  • ResourceList.Item

withRef

  • DropZone.FileUpload
  • UnstyledLink

@BPScott BPScott temporarily deployed to polaris-react-pr-805 December 21, 2018 18:23 Inactive
@BPScott
Copy link
Member

BPScott commented Dec 21, 2018

I'm not going to pretend to understand the TS intricacies that let this work but I love the sight of all those

- export default withAppProvider<Props>()(Thing);
+ export default withAppProvider()(Thing);

changes!

@BPScott BPScott temporarily deployed to polaris-react-pr-805 January 2, 2019 18:25 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-805 January 2, 2019 21:00 Inactive
@AndrewMusgrave AndrewMusgrave changed the title [WIP] Infer types in HoC Infer HoC props Jan 2, 2019
Updated types

Format

Fix withSticky
@chloerice
Copy link
Member

👀

export default function withAppProvider<OwnProps>() {
return function addProvider<C>(
export default function withAppProvider() {
return function addProvider<OriginalProps, C>(
Copy link
Contributor

Choose a reason for hiding this comment

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

C is Context in this case correct?

Copy link
Member Author

@AndrewMusgrave AndrewMusgrave Jan 3, 2019

Choose a reason for hiding this comment

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

I believe it's for inferring the typeof component. So for example

withSticky()(Scrollable);

will infer React.ComponentClass<Props, React.ComponentState> & typeof Scrollable. typeof Scrollable will be inferred from & C

Copy link
Contributor

@dleroux dleroux left a comment

Choose a reason for hiding this comment

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

This is great. What version of Typescript made this possible?

I couldn't build-consumer because of icons to test on web. I can't see see the resulting build impacting web but going with lessons learned.

@AndrewMusgrave
Copy link
Member Author

This is great. What version of Typescript made this possible?

Not sure

I couldn't build-consumer because of icons to test on web. I can't see see the resulting build impacting web but going with lessons learned.

I think it'll be best to wait as well, until we can 🎩 in web

@AndrewMusgrave
Copy link
Member Author

AndrewMusgrave commented Jan 3, 2019

Steps to 🎩 in web

You'll want to be on master in web and hoc-infer-types in polaris-react

# 1
dev cd web
# 2
dev down && dev up && yarn add -D @shopify/polaris-icons
# 3
dev cd polaris-react
# 4
dev up && yarn build-consumer web
# 5
dev cd web && dev server

Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

Tophatted in web and tested all components in the style guide as well. All are 💯

@AndrewMusgrave AndrewMusgrave merged commit 4fb6b73 into master Jan 4, 2019
@AndrewMusgrave AndrewMusgrave deleted the hoc-infer-types branch January 4, 2019 17:00
@danrosenthal danrosenthal temporarily deployed to production January 7, 2019 14:27 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.

5 participants