Skip to content
This repository was archived by the owner on Jun 29, 2026. It is now read-only.

feat: draft implementation of useStore in #733 (not production ready) - #734

Open
billyrrr wants to merge 3 commits into
yahoo:mainfrom
billyrrr:master
Open

feat: draft implementation of useStore in #733 (not production ready)#734
billyrrr wants to merge 3 commits into
yahoo:mainfrom
billyrrr:master

Conversation

@billyrrr

@billyrrr billyrrr commented Oct 7, 2021

Copy link
Copy Markdown

Co-authored-by: pablopalacios pablo.palacios@holidaycheck.com

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

…eady)

Co-authored-by: pablopalacios <pablo.palacios@holidaycheck.com>
@pablopalacios

Copy link
Copy Markdown
Contributor

Cool, let's go step by step. Important tests that we should have:

  • It registers/unregsiters to the given store
  • It updates when the store is updated

You can take a look at connectToStores tests as reference. As I mentioned in the issue, one thing that I'm concerned is the unregister part. We must make sure that when the component unmounts, we are removing the listener, otherwise we will have a memory leak.

Comment on lines +27 to +30
// useLayoutEffect is the closest to componentDidMount
// (we want to block render until store is subscribed)
// TODO: NOTE useLayoutEffect is called on server-side during SSR
useLayoutEffect(() => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need to use useLayoutEffect and not just useEffect?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The original test would fail for useEffect. I think that the following would take place with useEffect:

  1. render and return
  2. Test runs assertions on the return
  3. Actions in useEffect is triggered
    This causes the assertions in 2 to fail.

With useLayoutEffect:

  1. Render
  2. Actions in useLayoutEffect is executed and waited to be finished
  3. Test runs assertions on the return

Basically, it is just to ensure the same behavior as connectToStore.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Got it. Could you please take a look how react-redux is handling this issue: https://blog.isquaredsoftware.com/2018/11/react-redux-history-implementation/#connect-is-implemented-using-hooks
They are also using useLayoutEffect but on SSR they use useEffect to prevent the warning message.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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


expect(component.props.id).to.deep.equal('bar');
});
it('should register/unregister from stores on mount/unmount', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to make it simple to navigate between code blocks, I would recommend you to add one blank line between each test.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yep, I can do that. The IDE handles most of the reformatting though, so I might approach it by adding a lint rule.

@redonkulus

Copy link
Copy Markdown
Collaborator

@billyrrr @pablopalacios Still interested in this feature?

@pablopalacios

Copy link
Copy Markdown
Contributor

@redonkulus I do. At work we are using 2 custom hooks for stores: useStore and useStoreSubscription. The difference is that the later one subscribes to changes for the given store while the first not. We normally use the first when we are sure that the data is static (normally server side data). If this difference is not confusing you, I would propose to include both of them in the API (or, if you have a better idea...).

This is an example on how we use it:

function MyComponent() {
    const { locale } = useStore(AppStore).getState();
    const { count } = useStoreSubscription(CountStore).getState();

    return <span>Count: {new Intl.NumberFormat(locale).format(count)}</span>;
}

We have been using these hooks in production successfully since probably the time this PR was created. @billyrrr do you still have interested in moving the PR forward, otherwise I can prepare another one.

@billyrrr

Copy link
Copy Markdown
Author

Yep, I can continue to work on this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants