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

How to use with React Hooks #59

Closed
cayasso opened this issue Oct 28, 2018 · 33 comments
Closed

How to use with React Hooks #59

cayasso opened this issue Oct 28, 2018 · 33 comments

Comments

@cayasso
Copy link

cayasso commented Oct 28, 2018

What are your vision of using react-easy-state with the new and fresh React Hooks?
https://reactjs.org/docs/hooks-intro.html

@solkimicreb
Copy link
Member

No idea yet. I am on a holiday with very limited internet. I will catch up with React news in a week when I get back. (I guess there is a lot of news)

@cayasso
Copy link
Author

cayasso commented Oct 28, 2018

Yup lot of news! This is not a question only for this library but also for other state libraries like Mobx so I think by when your are back there might be some addition research from others on how to use, I will try to research my self.
So in the mean time enjoy your vacations ;-)

@cayasso
Copy link
Author

cayasso commented Oct 28, 2018

So this is what I have come up with so far:

import React, { useState, useEffect } from "react";
import ReactDOM from "react-dom";
import { store, view } from "react-easy-state";
import { observe, unobserve } from "@nx-js/observer-util";

const useEasyState = store => {
  const [state, setState] = useState(store);

  const handleStateChange = () => {
    console.log(state);
    setState({ ...state });
  };

  useEffect(
    () => {
      const fn = observe(handleStateChange);
      return () => unobserve(fn);
    },
    [store]
  );

  return state;
};

const counter = store({
  num: 0,
  inc: () => {
    return counter.num++;
  }
});

const Counter = () => {
  const { inc, num } = useEasyState(counter);
  return <button onClick={inc}>{num}</button>;
};

function App() {
  return (
    <div className="App">
      <Counter />
    </div>
  );
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);

The useEasyState does all the trick delegating the store value to the hook state it self and thats it, there might be some minor things that I am not considering but its a starting point. With this you can import the hook like this.

import { useEasyState } from 'react-easy-state'

Let me know what you think when you are back.

@cayasso
Copy link
Author

cayasso commented Oct 28, 2018

Here is the working example:
https://codesandbox.io/s/n49v8llw70

@penspinner
Copy link

Hi. Just wanted to show what I've been trying out.
https://codesandbox.io/s/m7124njn98

@solkimicreb
Copy link
Member

Hi All! I am back 🙂

Thanks for all the demos so far, I will catch up with them in the afternoon.

@solkimicreb
Copy link
Member

solkimicreb commented Nov 7, 2018

Thanks for the effort @cayasso and @penspinner. Here is my feedback/thoughts:

  • I think migrating internally to react-hooks without an external API change has no big benefit. The current React state will continue to be supported and the current perf is pretty good.

  • We could simplify the API down to single function much like @cayasso did. With hooks we wouldn't even need store, just the useStore function. (useStore could wrap the passed object with a reactive store since store wrapping is idempotent).

Something like:

import React from "react";
import ReactDOM from "react-dom";
import { useStore } from "react-easy-state";

const counter = {
  num: 0,
  inc: () => {
    return counter.num++;
  }
};

const Counter = () => {
  const { inc, num } = useStore(counter);
  return <button onClick={inc}>{num}</button>;
};

const rootElement = document.getElementById("root");
ReactDOM.render(<Counter />, rootElement);

There are two questions to consider:

  1. How to add this? Should we keep the current 2 function API and add this new function as a different way of usage?

  2. How to support local state?

Let me know what you think.

EDIT: I didn't have time to do a demo yet, so I am not a 100% sure that this is doable.

@ivan-kleshnin
Copy link

ivan-kleshnin commented Nov 7, 2018

My 5 cents.

How to add this? Should we keep the current 2 function API and add this new function as a different way of usage?

I'm fine with view + useStore as you proposed. Personally, I don't see a value in state methods (they are poorly composable) but that's another question.

How to support local state?

I would say state is "local" or "global" depending on the file it's located in... and that's all.
No extra technical differences are required.

@penspinner
Copy link

penspinner commented Nov 8, 2018

I think migrating internally to react-hooks without an external API change has no big benefit. The current React state will continue to be supported and the current perf is pretty good.

Yes. I know. It was cool to re-implement the view though. Who knows if hooks performance could be better than stateful class someday.

One thing I'm not sure about is would useStore(counter) create a global store? Would another component that useStore(counter) be re-rendered if the other component mutates counter properties?

import React from "react";
import ReactDOM from "react-dom";
import { useStore } from "react-easy-state";

const counter = {
  num: 0,
  inc: () => {
    return counter.num++;
  }
};

const Counter1 = () => {
  const { inc, num } = useStore(counter);
  return <button onClick={inc}>{num}</button>;
};

const Counter2 = () => {
  const { inc, num } = useStore(counter);
  return <button onClick={inc}>{num}</button>;
};

const App = () => (
  <>
    <Counter1 />
    <Counter2 />
  </>
);

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);

@cayasso
Copy link
Author

cayasso commented Nov 8, 2018

Welcome back @solkimicreb hope you enjoyed your vacations!

  • I pretty much agree with all that @ivan-kleshnin mentioned. view + useStore and that local vs global doesn't really matter in this case.

  • @solkimicreb in your example I don't think it is possible for useStore to wrap the object with a reactive store, because (and correct me if I am wrong) the object need a reference to the reactive object for example:

const counter = {
  num: 0,
  inc: () => {

    // won't work, it need a reference to "counterStore"
    return counter.num++;

    // will work because its a reference to reactive counterStore
    return counterStore.numm++;
  }
}

const counterStore = store(counter)

@andreasnilssondev
Copy link

Hi everyone. I have 2 questions here (and one big thank you for the amazing library!).

  1. I want to use a global state variable in the same component as I use local state with useState. However it's not possible to use the view HOC together with useState. Is this what this issue is addressing? (sorry if I'm in the wrong place here)
    The error is Hooks can only be called inside the body of a function component. Which I guess makes sense, since the HOC is a class. See this example (Try adding the view HOC in App.jsx and it will break): https://codesandbox.io/s/6jkx4nj7ow

  2. I'm slightly worried about API changes (probably because I don't fully understand what the examples mean for our usecase). We normally use it like this:

// globalstate.js
import { store } from 'react-easy-state';

const globalstate = store({
  number: 0
});

export default globalstate;
// MyComponent.js
import React from 'react';
import globalstate from './globalstate';

function MyComponent() {
  return <p>my number is {globalstate.number}</p>;
}

export default view(MyComponent);

Would it still be possible to import globalstate as the state variable after the suggested changes? Or would we have to change to using useStore inside the components? Sometimes we use the values outside react components too. Maybe it's too early for these questions, I'm really just worried about the API changing too much (a lot of refactoring) 😃

@ivan-kleshnin
Copy link

@nAndreas the update is not released yet. Current easy-state will indeed cause that error.

I'm really just worried about the API changing too much (a lot of refactoring)

To my understanding, everything is 100% backward compatible. It's only if you move to hooks in your code base – you'll need to apply this new API.

@solkimicreb
Copy link
Member

I did some research and everything I wrote so far was more or less incorrect, sorry about that. Now I have a tiny working demo to back up the ideas. Big thanks @penspinner for your demo! My one is a slightly modified version of yours.

I hoped to shrink the current API to a single hook, but it turned out that we have to keep both the store (as @cayasso pointed out) and the view function. The good news is that we can continue to support all use cases with the current API at least.

This is the proposed API for the new use cases.

  • The new version will work with pre-react-16.7 class and function components. (No breaking changes)
  • It will work together with hooks and support local state in React 16.7+ function components.
import React from 'react';
import ReactDOM from 'react-dom';
import { view, store } from 'react-easy-state';

// this is a global store which is shared between component instances
const globalStore = store({ show: true })

const toggleShow = () => (globalStore.show = !globalStore.show)

// using store inside a function component creates a local store that is not shared between component instances
const Counter = view(() => {
  const localStore = store({ num: 12 })
  const inc = () => localStore.num++

  return <button onClick={inc}>{localStore.num}</button>
});

const App = view(() => (
  <div>
    {globalStore.show && <Counter />}
    <button onClick={toggleShow}>Toggle Counter</button>
  </div>
))

ReactDOM.render(<App />, document.getElementById('root'));

You can find the codesandbox demo with the initial implementation here.

Let me know what you think.

@cayasso
Copy link
Author

cayasso commented Nov 9, 2018

@solkimicreb It looks great to me, I like the fact that we continue using the same API, there is one change only, I noticed that in easyState.js you have this line scheduler: setState,, this line like that will cause the following error when using regular react hooks in a function with view(...):

TypeError: Cannot read property '0' of undefined

You can reproduce the error here https://codesandbox.io/s/l5nkkqqp0l notice what happen when you increment.

The solution is to use scheduler: () => setState(), as in @penspinner initial code.

@penspinner
Copy link

penspinner commented Nov 10, 2018

@solkimicreb

Just tried your example. Looks fine except for a few things.

  • When pressing the Toggle Counter button, "App" is logged to the console twice. Not sure why. Is it supposed to do that?
  • I noticed the store function is using useMemo within an if statement. The React team emphasized that hooks should be at the top level of a React function at all times to ensure things work as expected. It seems to work fine in your example, but not sure if it would function correctly all the times. https://reactjs.org/docs/hooks-rules.html

@solkimicreb
Copy link
Member

solkimicreb commented Nov 17, 2018

I hope to have some time over the weekend to implement this.

Thanks for the feedback @penspinner

When pressing the Toggle Counter button, "App" is logged to the console twice. Not sure why. Is it supposed to do that?

This is a bug, I will look into it.

I noticed the store function is using useMemo within an if statement. The React team emphasized that hooks should be at the top level of a React function at all times to ensure things work as expected. It seems to work fine in your example, but not sure if it would function correctly all the times. https://reactjs.org/docs/hooks-rules.html

This won't be an issue. Every hook must be called in the exact same order on every render. (This is a lower level equivalent oh the 'hook rules'). My if block is doing exactly this, it calls useMemo when store is invoked from inside a render and does not call it otherwise. Without the if block we would actually break react because of the hooks called outside from renders. I will make sure to comment the final code about this.

EDIT: this might take some time. I think I will take the opportunity and rewrite enzyme tests. (Enzyme lags behind the latest React features.)

EDIT2: updated and fixed codesandbox demo: https://codesandbox.io/s/oq66nmyjpz

EDIT3: This is also leading me into refactoring the examples and tests: facebook/react#13991 (npm link is not working properly with React hooks)

@solkimicreb
Copy link
Member

solkimicreb commented Nov 22, 2018

Hooks update (v6.1.0-beta.0) is ready, you can try the beta release with npm i react-easy-state@beta. The new code and readme is on this branch. Everything is as discussed in the final comments.

Please play with it a bit if you have some time. I don't expect to add any breaking changes, so don't be afraid to try it out. Thanks everyone for the help so far!

@cayasso
Copy link
Author

cayasso commented Nov 23, 2018

@solkimicreb thank you for taking the time and implementing this!

@ivan-kleshnin
Copy link

ivan-kleshnin commented Nov 23, 2018

I'm seeing a difference in how batching works in React@next + Easy State@next comparing to React@16.6 + EasyState@current.

Not sure which one is caused that, but:

export async function fetchModels([tableName, ids]) {
  state.loading += 1 // !!! this previously caused components to rerender with <Loading/>
  // !!! now it's batched, previous behavior was what I wanted

  let apiUrl = `/api/${tableName}/id`
  let reqResult = await fetchJSON(apiUrl, {
    method: "SEARCH",
    body: ids,
  })

  if (reqResult instanceof Error) {
    return handleHttpError(apiUrl, reqResult)
  } else {
    let models = reqResult.models
    batch(() => {
      state.loading -= 1
      state[tableName] = R.merge(state[tableName], models)
    })
  }
}

Anyone experiencing the same? It there a way to disable this kind of batching?

@ivan-kleshnin
Copy link

This were somehow caused by withRouter:

view(withRouter(Layout)) // worked previously
->
withRouter(view(Layout)) // works now

No time to investigate, m.b. will help someone.

@solkimicreb
Copy link
Member

Thanks for the catch, the second comment helped a lot! Which version of React Router are you using?

@solkimicreb
Copy link
Member

Side note: this is (most probably) not a batching issue. In fact, you don't need to manually batch in your case. Auto batching was also improved in v.6.1.0, it covers pretty much all practical use cases. (Release notes coming with the stable update.) Batching manually has no drawbacks though, so if you don't mind the slightly ugly syntax you can keep them to be 100% sure.

@ivan-kleshnin
Copy link

ivan-kleshnin commented Nov 23, 2018

Which version of React Router are you using?

Tried with 4.3.0 and 4.4.beta

this is (most probably) not a batching issue

The initial reason to think "it's batching" for me was that only state.loading = update was skipped / lost while state.models = had effect indeed.

But as the change of composition order affects the behavior, it's probably not batching or not only batching – I agree on that.

@solkimicreb
Copy link
Member

Tried with 4.3.0 and 4.4.beta

4.4.beta is using the new context api and I hoped it would eliminate this issue. I will add some React Router interaction tests and look into this before the next release. Thanks for the info again.

@solkimicreb
Copy link
Member

@ivan-kleshnin Are you sure it was working with view(withRouter(Layout)) previously. I just did some tests with the current and the older versions and both work correctly with withRouter(view(Layout)) and fail with view(withRouter(Layout)). This is also the "expected behavior" when you combine some other HOCs with react router.

I am asking for a double check because I don't want to include accidental breaking changes.

@ivan-kleshnin
Copy link

ivan-kleshnin commented Nov 26, 2018

Are you sure it was working with view(withRouter(Layout)) previously

Yes, absolutely. Otherwise I wouldn't notice this change. I would estimate the probability that it was "something else affecting the impression" as extremely low in this case. Still it could be some weird combination of prop names, etc that made it work when it wouldn't for general case.

@solkimicreb
Copy link
Member

solkimicreb commented Nov 26, 2018

Thanks! Did anyone else run into this issue with the beta?

Edit: the issue is a change in behavior when used together with react-router

@ivan-kleshnin
Copy link

Rechecked once again, and I see no problems with easy-state@6.1.0-beta.0, react@next and react-router@next. All possible sequences of decorators work equally fine in my cases.

@andreasnilssondev
Copy link

Hi @solkimicreb,

I would love to try this out in my project and let you know how it works, but I need the latest bugfixes on master for my project to work.

Whenever you have time, would it be possible to do a second beta release with the fixes from master 😄

@solkimicreb
Copy link
Member

Sure, I will try to do it soon. I was pretty busy recently.

@solkimicreb
Copy link
Member

Hi all, the latest beta is out. You can try it with npm i react-easy-state@beta. It adds StrictMode compatibility, better batching and a few fixes. Changelog and article will be included with the stable release, which will hopefully happen very soon.

@solkimicreb
Copy link
Member

v6.1.0 (stable) is released with hooks support among others. Short article is here, changelog is here.

I am closing the issue now, thanks for the input and help everyone! 🙂

@cayasso
Copy link
Author

cayasso commented Jan 31, 2019

Thank you @solkimicreb!

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

No branches or pull requests

5 participants