Try/state in context use hook #40

EricRibeiro · 2021-04-17T16:52:02Z

Resolves: #39

Benefits

Less code, no need to spread those states everywhere
Better performance, less rerenders

Goals

100% backward compatibility. It should work with every existing component without any changes. This isn't proved yet but I'll include tests later on.
Should be easy to enable this new feature for existing and upcoming components

Is there anything we need to do to enable this feature for components?

Yes

The PR introduces two new hooks: useStateContextConsumer and useStateContextProvider. To create a context you can use the helper function createStateContext.

`useStateContextProvider`

This hook should be used by the parent component. For example ComboboxPopover is a component that contains ComboboxOption. In this case, ComboboxPopover is the parent component and it should provide the context.

First, you will need a context:

export const StateContext = createStateContext<
  Partial<unstable_ComboboxState>
>();

Then you need to add the hook to the compose array as the very first value:

compose: [useStateContextProvider(StateContext), useComboboxList, usePopover],

If your component is using useComposeProps too, then you need to add the hook there as well:

useComposeProps(options, { tabIndex, ...htmlProps }) {
  htmlProps = useStateContextProvider(StateContext)(options, htmlProps, true);
  htmlProps = useComboboxList(options, htmlProps, true);
  htmlProps = usePopover(options, htmlProps, true);

  return {
    ...htmlProps,
    tabIndex: tabIndex ?? undefined,
  };
}

`useStateContextConsumer`

This hook should be used by the child component. For example ComboboxPopover is a component that contains ComboboxOption. In this case, ComboboxOption is the child component and it should consume the context.

You should create a context in your parent component as mentioned above. Then you need to add the hook to the compose array as the very first value:

compose: [
  useStateContextConsumer({
    context: StateContext,
    shouldUpdate: (id, state, nextState) => {
      return (
        state.currentId === null ||
        nextState.currentId === null ||
        id === state.currentId ||
        id === nextState.currentId
      );
    },
    updateDependencies: (state) => [state?.currentId],
  }),
  useComboboxItem,
  useCompositeItem,
]

useStateContextConsumer takes an object with 3 properties:

context the context that you would like to consume, this should be the context that your parent component provides
shouldUpdate a function that returns whether the component should update its state or not. This function gives you 3 parameters: id of the component, state current state of the component and nextState which is the state that we just received from the context. If we return true, then the nextState will be applied to the component.
updateDependencies under the hood, the pubsub system takes some dependencies. According to these dependencies, your component resubscribes to the pubsub system. This makes sure you receive correct values in shouldUpdate.

How to test?

npm run storybook
Open Combobox -> Combobox Non Context Vs Context

Does this PR introduce breaking changes?

Hopefully not!

…akit/reakit into try/state-in-context-use-hook

todo · 2021-04-17T16:52:06Z

implement unit test

red-experiment/packages/reakit/src/Composite/__utils/fillGroups.ts

Lines 14 to 19 in dc33f3c

    
           // TODO: implement unit test 
        
           export function createEmptyItems(size: number, groupId?: string) { 
        
             const items = []; 
        
             for (let i = 0; i++; i < size) { 
        
               items.push({

This comment was generated by todo based on a `TODO` comment in `dc33f3c` in #40. cc @the-funnel.

todo · 2021-04-17T16:52:09Z

define function's return type

red-experiment/packages/reakit/src/Composite/__utils/findFirstEnabledItem.ts

Lines 10 to 15 in dc33f3c

    
           // TODO: define function's return type 
        
           export function findLastEnabledItem(items: Item[], excludeId?: string) { 
        
             if (excludeId) { 
        
               return items 
        
                 .reverse() 
        
                 .find((item) => !item.disabled && item.id !== excludeId);

This comment was generated by todo based on a `TODO` comment in `dc33f3c` in #40. cc @the-funnel.

todo · 2021-04-17T16:52:12Z

implement unit test

red-experiment/packages/reakit/src/Composite/__utils/getItemsInGroup.ts

Lines 7 to 10 in dc33f3c

    
           // TODO: implement unit test 
        
           export function getDisabledItems(items: Item[]) { 
        
             return items.filter((item) => item.disabled); 
        
           }

This comment was generated by todo based on a `TODO` comment in `dc33f3c` in #40. cc @the-funnel.

todo · 2021-04-17T16:52:15Z

define this function return type

red-experiment/packages/reakit/src/Composite/__utils/groupItems.ts

Lines 3 to 6 in dc33f3c

    
           // todo: define this function return type 
        
           export function groupItems(items: Item[]) { 
        
             const groups = [[]] as Item[][];

This comment was generated by todo based on a `todo` comment in `dc33f3c` in #40. cc @the-funnel.

EricRibeiro · 2021-04-17T16:52:55Z

@RonanFelipe This is an alternative implementation that is not hacking into the createComponent. I would like to avoid that to make it more reusable and robust. This way we can totally decouple the StateContext and all the related components.

And it is using createHook so it's more Reakit-ish way.

codesandbox-ci · 2021-04-17T16:57:38Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit dc33f3c:

Sandbox	Source
Reakit	Configuration

EricRibeiro · 2021-04-17T16:57:44Z

@RonanFelipe Just finished this up 😄 Tests are passing. I'll experiment with other components as well. Would like to hear your thoughts about this implementation.

github-actions · 2021-04-17T17:00:47Z

Size Change: +1.15 kB (0%)

Total Size: 258 kB

Filename	Size	Change
`packages/reakit-playground/dist/reakit-playground.min.js`	187 kB	+42 B (0%)
`packages/reakit/dist/reakit.min.js`	37.1 kB	+1.1 kB (+3%)

ℹ️ View Unchanged

Filename	Size	Change
`packages/reakit-system-bootstrap/dist/reakit-system-bootstrap.min.js`	19.1 kB	0 B
`packages/reakit-system-palette/dist/reakit-system-palette.min.js`	8.85 kB	0 B
`packages/reakit-system/dist/reakit-system.min.js`	2.45 kB	0 B
`packages/reakit-utils/dist/reakit-utils.min.js`	3.46 kB	0 B

_{compressed-size-action}

RonanFelipe · 2021-04-17T17:00:56Z

Amazing work @EricRibeiro 🚀

I currently don't have time to review Reakit PRs (I work on react native right now and am currently trying to fight off burnout in these trying times, so avoiding out of hours projects) - I've removed myself from the review request - hope you don't mind!

codecov · 2021-04-17T17:01:48Z

Codecov Report

Merging #40 (b56d994) into master (4c44464) will decrease coverage by 0.63%.
The diff coverage is 66.26%.

❗ Current head b56d994 differs from pull request most recent head dc33f3c. Consider uploading reports for the commit dc33f3c to get more accurate results

@@            Coverage Diff             @@
##           master      #40      +/-   ##
==========================================
- Coverage   95.12%   94.48%   -0.64%     
==========================================
  Files         230      236       +6     
  Lines        3509     3592      +83     
  Branches      955      968      +13     
==========================================
+ Hits         3338     3394      +56     
- Misses        170      196      +26     
- Partials        1        2       +1

Impacted Files	Coverage Δ
...__examples__/ComboboxNonContextVsContext/index.tsx	`0.00% <0.00%> (ø)`
...ackages/reakit/src/Composite/__utils/fillGroups.ts	`75.00% <0.00%> (-25.00%)`	⬇️
...akit/src/Composite/__utils/findFirstEnabledItem.ts	`44.44% <0.00%> (-55.56%)`	⬇️
...es/reakit/src/Composite/__utils/getItemsInGroup.ts	`50.00% <0.00%> (-50.00%)`	⬇️
...ackages/reakit/src/Composite/__utils/groupItems.ts	`100.00% <ø> (ø)`
...ages/reakit/src/StateContext/createStateContext.ts	`66.66% <66.66%> (ø)`
...eakit/src/StateContext/useStateContextConsumer.tsx	`90.00% <90.00%> (ø)`
...eakit/src/StateContext/useStateContextProvider.tsx	`95.45% <95.45%> (ø)`
packages/reakit/src/Combobox/ComboboxOption.ts	`100.00% <100.00%> (ø)`
packages/reakit/src/Combobox/ComboboxPopover.ts	`100.00% <100.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c44464...dc33f3c. Read the comment docs.

david-szabo97 and others added 27 commits February 9, 2021 14:36

Try context

87d73e1

Styling

f26d929

Typing

b39e552

Add createStateContext

5d20394

Remove HOCInner

3025ac7

Use createStateContext

d86f53a

Rename hoc to withStateContextSubscriber

7544049

Fix search in story

65acc9c

Try use hook based context

0ab1237

Cleanup

c800ef7

Rename

e5e1055

Cleanup story

6292bf9

Restore story

2ae46a7

Fix error

abf42d6

Move to own folder

bc67d4d

Typing

b908a81

Update name

05c0000

Add shouldUpdate and updateDeps

2eecaa7

Remove old export

03b684d

Fix

ac22b28

Update

ad90600

Add story

50eaa29

Remove old code

985f9df

Remove old code

171726f

Checkin build files

3937cf9

Merge branch 'try/state-in-context-use-hook' of https://github.com/re…

068f5a9

…akit/reakit into try/state-in-context-use-hook

add new utilitary functions

dc33f3c

EricRibeiro requested review from mairieli and RonanFelipe April 17, 2021 16:52

RonanFelipe removed their request for review April 17, 2021 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try/state in context use hook #40

Try/state in context use hook #40

EricRibeiro commented Apr 17, 2021

todo bot commented Apr 17, 2021

todo bot commented Apr 17, 2021

todo bot commented Apr 17, 2021

todo bot commented Apr 17, 2021

EricRibeiro commented Apr 17, 2021

codesandbox-ci bot commented Apr 17, 2021

EricRibeiro commented Apr 17, 2021

github-actions bot commented Apr 17, 2021

RonanFelipe commented Apr 17, 2021

codecov bot commented Apr 17, 2021

Try/state in context use hook #40

Are you sure you want to change the base?

Try/state in context use hook #40

Conversation

EricRibeiro commented Apr 17, 2021

Benefits

Goals

Is there anything we need to do to enable this feature for components?

useStateContextProvider

useStateContextConsumer

How to test?

Does this PR introduce breaking changes?

todo bot commented Apr 17, 2021

implement unit test

This comment was generated by todo based on a TODO comment in dc33f3c in #40. cc @the-funnel.

todo bot commented Apr 17, 2021

define function's return type

This comment was generated by todo based on a TODO comment in dc33f3c in #40. cc @the-funnel.

todo bot commented Apr 17, 2021

implement unit test

This comment was generated by todo based on a TODO comment in dc33f3c in #40. cc @the-funnel.

todo bot commented Apr 17, 2021

define this function return type

This comment was generated by todo based on a todo comment in dc33f3c in #40. cc @the-funnel.

EricRibeiro commented Apr 17, 2021

codesandbox-ci bot commented Apr 17, 2021

EricRibeiro commented Apr 17, 2021

github-actions bot commented Apr 17, 2021

RonanFelipe commented Apr 17, 2021

codecov bot commented Apr 17, 2021

Codecov Report

`useStateContextProvider`

`useStateContextConsumer`

This comment was generated by todo based on a `TODO` comment in `dc33f3c` in #40. cc @the-funnel.

This comment was generated by todo based on a `TODO` comment in `dc33f3c` in #40. cc @the-funnel.

This comment was generated by todo based on a `TODO` comment in `dc33f3c` in #40. cc @the-funnel.

This comment was generated by todo based on a `todo` comment in `dc33f3c` in #40. cc @the-funnel.