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

Try state in context with useHook #796

Closed
wants to merge 25 commits into from

Conversation

david-szabo97
Copy link
Contributor

@david-szabo97 david-szabo97 commented Nov 27, 2020

Resolves: #745

Alternative implementation to #792, I felt like #792 more of a hack rather than a solution. That's why I moved on and tried to come up with a solution that doesn't change the core and more reusable.

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!

@ariakit-bot
Copy link

Deploy preview for reakit ready!

Built with commit 8dfd5e4

https://deploy-preview-796--reakit.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented Nov 27, 2020

Size Change: +1.15 kB (0%)

Total Size: 258 kB

Filename Size Change
packages/reakit-playground/dist/reakit-playground.min.js 187 kB +44 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

@david-szabo97
Copy link
Contributor Author

@diegohaz 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.

@ariakit-bot
Copy link

Deploy preview for reakit ready!

Built with commit 22fe86c

https://deploy-preview-796--reakit.netlify.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 27, 2020

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 3937cf9:

Sandbox Source
Reakit Configuration

@ariakit-bot
Copy link

ariakit-bot commented Nov 27, 2020

Deploy preview for reakit ready!

Built with commit 3937cf9

https://deploy-preview-796--reakit.netlify.app

@david-szabo97 david-szabo97 force-pushed the try/state-in-context-use-hook branch from 3f64099 to 50eaa29 Compare February 9, 2021 13:36
@codecov
Copy link

codecov bot commented Feb 9, 2021

Codecov Report

Merging #796 (3937cf9) into master (9a59d99) will decrease coverage by 0.32%.
The diff coverage is 77.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #796      +/-   ##
==========================================
- Coverage   95.12%   94.80%   -0.33%     
==========================================
  Files         231      237       +6     
  Lines        3510     3581      +71     
  Branches      955      967      +12     
==========================================
+ Hits         3339     3395      +56     
- Misses        170      184      +14     
- Partials        1        2       +1     
Impacted Files Coverage Δ
...__examples__/ComboboxNonContextVsContext/index.tsx 0.00% <0.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%> (ø)
...x/__examples__/AccessibleComboboxContext/index.tsx 100.00% <100.00%> (ø)
...ackages/reakit/src/StateContext/useStateContext.ts 100.00% <100.00%> (ø)
... and 5 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 9a59d99...3937cf9. Read the comment docs.

@david-szabo97 david-szabo97 marked this pull request as ready for review February 11, 2021 11:31
@david-szabo97
Copy link
Contributor Author

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

@tom-sherman tom-sherman removed their request for review February 11, 2021 11:34
@tom-sherman
Copy link
Member

tom-sherman commented Feb 11, 2021

Amazing work @david-szabo97 🚀

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!

@brianespinosa
Copy link

This looks pretty slick, and definitely way better for userland in general. I will give the tires a kick on this on our functional design system sometime next week to see how it feels.

@diegohaz
Copy link
Member

@brianespinosa Thanks! That will help a lot. I'm more focused on v2 right now, but it would be awesome to have this feature on v1 as well. David has done an excellent job.

@brianespinosa
Copy link

@diegohaz do we have an alpha or nightly build version for v2 that we can take a look at yet? I did not realize that was coming.

@diegohaz
Copy link
Member

@diegohaz do we have an alpha or nightly build version for v2 that we can take a look at yet? I did not realize that was coming.

Not yet! I'm still experimenting with some different implementations to see if/how they work in real projects, especially considering the new concurrent rendering features on React. Once I have something functional I'll create a branch here and propose the changes. I expect this will take a few months, but I hope to have an alpha version or something by the end of the year.

@stale
Copy link

stale bot commented Jan 14, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 14, 2022
@stale stale bot closed this Jan 23, 2022
@diegohaz diegohaz deleted the try/state-in-context-use-hook branch January 23, 2022 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Hybrid implicit/explicit state
5 participants