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

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

billyrrr
Copy link

@billyrrr billyrrr commented Oct 7, 2021

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

@billyrrr @pablopalacios Still interested in this feature?

@pablopalacios
Copy link
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
Author

Yep, I can continue to work on this issue.

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.

None yet

3 participants