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

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

EricRibeiro
Copy link
Member

Resolves: #6

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!

@ghost ghost deleted a comment from todo bot Apr 18, 2021
@ghost ghost deleted a comment from todo bot Apr 18, 2021
@ghost ghost deleted a comment from todo bot Apr 18, 2021
@EricRibeiro
Copy link
Member Author

EricRibeiro commented Apr 18, 2021

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

@ghost
Copy link

ghost commented Apr 18, 2021

Hi @the-funnel

You have 9 comments made by bots installed on this repo regarding this pull request. Check them below:

There are 7 critical comments
There is 1 deploy comment
  • 1 comment made by codesandbox[bot]

    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 266f295:

    Sandbox Source
    Reakit Configuration
There is 1 info comment
  • 1 comment made by github-actions[bot]

    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

@ghost ghost deleted a comment from github-actions bot Apr 18, 2021
@ghost ghost deleted a comment from todo bot Apr 18, 2021
@ghost ghost deleted a comment from todo bot Apr 18, 2021
@ghost ghost deleted a comment from todo bot Apr 18, 2021
@ghost ghost deleted a comment from codesandbox-ci bot Apr 18, 2021
@ghost ghost deleted a comment from codecov bot Apr 18, 2021
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.

RFC: Hybrid implicit/explicit state
2 participants