Support for jsx3 with hooks #45

MargaretKrutikova · 2019-07-13T16:18:13Z

This PR addresses #44 and adds support for using React.Context with store and components written in JSX3 with hooks.

What this PR does:

adds support for using React.Context and exposes Provider that uses store as value,
adds useSelector hook for querying state and useDispatch for accessing dispatch,
converts all examples to JSX3, and keeps one example with JSX2 for backwards compatibility,
updates bs-platform and react dependencies and moves reason-react from dependencies to peerDependencies.

nlfiedler · 2019-07-13T22:44:30Z

Terrific, and it's backward compatible, too! Thanks

MargaretKrutikova · 2019-07-13T23:32:34Z

No problem! I also want to add some tests to cover the useSelector hook, similar to what they have in react-redux.

MargaretKrutikova · 2019-07-14T13:24:40Z

I added tests for useSelector, the only problem was a lot of warnings from the test libraries which I wasn't able to figure out how to remove (might be something with the bindings...).
I would appreciate any suggestions/code reviews/feedback on the PR 🙂

rickyvetter

Thanks for working on this! The implementation seems much more complicated than I expected, which makes me a bit nervous. Could very well be that I haven't thought of the edge cases this PR covers fully though. Left some inline comments with more questions!

rickyvetter · 2019-07-15T14:54:30Z

.gitignore

@@ -7,3 +7,5 @@
 npm-debug.log
 /_build/
 package-lock.json
+*.bs.js


I'm not sure it makes sense to ignore the generated output. Can be useful to see it in code reviews and for verification.

Agree! Fixed.

rickyvetter · 2019-07-15T15:04:35Z

src/reductiveContext.re

+    };
+  };
+
+  let useSelector = selector => {


~~This function seems massively complex and I'm not really sure why. As far as I can tell we just want to memoize selector+storeFromContext and get the resulting view of state?~~

~~Would something like this work?~~

Ignore all of this. I wasn't thinking things through :)

rickyvetter · 2019-07-15T15:07:43Z

src/reductiveContext.re

+        React.Ref.setCurrent(latestSelector, selector);
+
+        let newSelectedState =
+          selector(Reductive.Store.getState(Config.store));


This function uses Config.store in a couple places - why does it do that instead of using storeFromContext?

My mistake! Fixed it in the new implementation of useSelector.

rickyvetter · 2019-07-15T15:10:23Z

src/reductiveContext.re

+    let latestSelector = React.useRef(selector);
+
+    // selector function might change if using components' props to select state
+    React.useLayoutEffect1(


We should really try to avoid using useLayoutEffect if we can. It's not really intended for this kind of general state management usecase.

The new implementation based on useSubscription is using useEffect instead.

rickyvetter · 2019-07-15T15:12:28Z

src/reductiveContext.rei

+        "";
+      let make: {. "children": React.element} => React.element;
+    };
+    let useSelector: (Config.state => 'a) => 'a;


Where possible it'd be great to use more specific type variable names since they show up in editor tooling. Maybe something like 'selectedState would be appropriate here?

rickyvetter · 2019-07-15T15:13:54Z

src/reductiveContext.re

+
+    let currentStoreState = Reductive.Store.getState(Config.store);
+    if (React.Ref.current(latestSelector) !== selector) {
+      selector(currentStoreState);


I think you'd want to set latestSelectedState here, no?

rickyvetter · 2019-07-15T15:14:04Z

src/reductiveContext.re

+      selector(currentStoreState);
+    } else {
+      switch (React.Ref.current(latestSelectedState)) {
+      | None => selector(currentStoreState)


rickyvetter · 2019-07-15T15:14:56Z

__tests__/reductiveContext_test.re

@@ -0,0 +1,224 @@
+[@bs.config {jsx: 3}];


🔥

Thanks for the tests!

rickyvetter · 2019-07-15T15:20:53Z

Interesting. I see this code is based off of https://github.com/reduxjs/react-redux/blob/master/src/hooks/useSelector.js which makes a ton of sense - looking into why that code is written that way.

rickyvetter · 2019-07-15T15:47:03Z

Okay I think it would be better to base this work off of https://github.com/facebook/react/pull/15022/files as opposed to the existing react-redux stuff. This approach is much more in-line with the future of concurrent mode and I think the resulting code will be much easier to work with as well. You can imagine a memoized version of selector and a subscribe function being passed as inputs into this hook.

MargaretKrutikova · 2019-07-15T19:39:32Z

Thanks a lot for the review! I totally agree that the implementation got quite complicated.

By basing this work off of useSubscription hook, do you mean taking inspiration from its implementation or actually using the hook when it gets merged into react?

rickyvetter · 2019-07-16T13:46:20Z

I was thinking about having a 1:1 translation inline for now and then swapping out once it’s shipped at https://www.npmjs.com/package/use-subscription

Open to other strategies though!

MargaretKrutikova · 2019-07-16T23:33:48Z

The useSubscription hook just got merged into master, but there is this section that was added to README:

When should you NOT use this?
This utility should be used for subscriptions to a single value that are typically only read in one place and may update frequently (e.g. a component that subscribes to a geolocation API to show a dot on a map).

Other cases have better long-term solutions:

Redux/Flux stores should use the context API instead.

I am concerned about two things here: a scenario where the value is only read in one place seems not applicable for a large redux application, where there are usually quite a few components subscribed to the store, and the better long-term solution with useContext.

useContext alone doesn't seem to solve the problem with components re-rendering more than necessary, which eventually might lead to performance issues (reported in react-redux v6 where context was used to propagate state changes).

Would you still advice using the implementation from useSubscription?

tizoc · 2019-07-17T01:20:15Z

NOTE: the code here is 100% untested, just quickly written to show one of the possibilities and continue the discussion.

A very rough implementation of useSelector using useSubscription (defined below):

let useSelector = (selector) => {
  let store = React.useContext(storeContext);
  let subscribe =
    React.useCallback1(
      handler => Reductive.Store.subscribe(store, handler),
      [|store|],
    );
  let getCurrentValue =
    React.useCallback1(
      () => selector(Reductive.Store.getState(store)),
      [|store|],
    );
  useSubscription(getCurrentValue, subscribe);
};

And a very rough translation of useSubscription(note that the function parameters change, instead of the "subscription" object the two parameters are passed separately):

type subscriptionState('a) = {
  getCurrentValue: unit => 'a,
  subscribe: (unit => unit) => (unit => unit),
  value: 'a,
};

// Hook used for safely managing subscriptions in concurrent mode.
//
// In order to avoid removing and re-adding subscriptions each time this hook is called,
// the parameters passed to this hook should be memoized in some way–
// either by wrapping the entire params object with useMemo()
// or by wrapping the individual callbacks with useCallback().
let useSubscription =
    (
      // (Synchronously) returns the current value of our subscription.
      getCurrentValue,

      // This function is passed an event handler to attach to the subscription.
      // It should return an unsubscribe function that removes the handler.
      subscribe,
    ) => {
  // Read the current value from our subscription.
  // When this value changes, we'll schedule an update with React.
  // It's important to also store the hook params so that we can check for staleness.
  // (See the comment in checkForUpdates() below for more info.)
  let (state, setState) =
    React.useState(() =>
      {getCurrentValue, subscribe, value: getCurrentValue()}
    );

  let valueToReturn = ref(state.value);

  // If parameters have changed since our last render, schedule an update with its current value.
  if (state.getCurrentValue !== getCurrentValue
      || state.subscribe !== subscribe) {
    // If the subscription has been updated, we'll schedule another update with React.
    // React will process this update immediately, so the old subscription value won't be committed.
    // It is still nice to avoid returning a mismatched value though, so let's override the return value.
    valueToReturn := getCurrentValue();

    setState(_ => {getCurrentValue, subscribe, value: valueToReturn^});
  };

  // Display the current value for this hook in React DevTools.
  // TODO: useDebugValue does not exist in Reason-React
  // React.useDebugValue(valueToReturn^);

  // It is important not to subscribe while rendering because this can lead to memory leaks.
  // (Learn more at reactjs.org/docs/strict-mode.html#detecting-unexpected-side-effects)
  // Instead, we wait until the commit phase to attach our handler.
  //
  // We intentionally use a passive effect (useEffect) rather than a synchronous one (useLayoutEffect)
  // so that we don't stretch the commit phase.
  // This also has an added benefit when multiple components are subscribed to the same source:
  // It allows each of the event handlers to safely schedule work without potentially removing an another handler.
  // (Learn more at https://codesandbox.io/s/k0yvr5970o)
  React.useEffect2(
    () => {
      let didUnsubscribe = ref(false);

      let checkForUpdates = () =>
        // It's possible that this callback will be invoked even after being unsubscribed,
        // if it's removed as a result of a subscription event/update.
        // In this case, React will log a DEV warning about an update from an unmounted component.
        // We can avoid triggering that warning with this check.
        if (! didUnsubscribe^) {
          setState(prevState =>
            // Ignore values from stale sources!
            // Since we subscribe an unsubscribe in a passive effect,
            // it's possible that this callback will be invoked for a stale (previous) subscription.
            // This check avoids scheduling an update for that stale subscription.
            if (prevState.getCurrentValue !== getCurrentValue
                || prevState.subscribe !== subscribe) {
              prevState;
            } else {
              // Some subscriptions will auto-invoke the handler, even if the value hasn't changed.
              // If the value hasn't changed, no update is needed.
              // Return state as-is so React can bail out and avoid an unnecessary render.
              let value = getCurrentValue();
              if (prevState.value === value) {
                prevState;
              } else {
                {...prevState, value};
              };
            }
          );
        };

      let unsubscribe = subscribe(checkForUpdates);

      // Because we're subscribing in a passive effect,
      // it's possible that an update has occurred between render and our effect handler.
      // Check for this and schedule an update if work has occurred.
      checkForUpdates();

      Some(
        () => {
          didUnsubscribe := true;
          unsubscribe();
        },
      );
    },
    (getCurrentValue, subscribe),
  );

  valueToReturn^;
};

tizoc · 2019-07-19T15:11:11Z

Should the user be expected to memoize the selector functions passed to useSelector?

I ran @MargaretKrutikova's tests (thank you for writing those btw) with the implementation I pasted above and the test that checks that changing props are handled gracefully fails: https://github.com/reasonml-community/reductive/pull/45/files#diff-81724037f70299d409a468210e14b0b8R139

It fails because the implementation I provided memoizes the function and assumes selector will not change. Of course, this is not true when the selector depends on props. But if that useSelector implementation takes this into account, then each time it is called (unless the user memoizes the selector) the selector function is different, even if does exactly the same as in previous renders (new closure with different identity).

Because useSubscription stores the subscribe and getCurrentValue functions using setState, each time the selector function changes (which is always if the user doesn't memoize), setState gets called again (because getCurrentValue depends on the selector function), and this causes a lot of unnecessary re-renders.

MargaretKrutikova · 2019-07-19T15:45:30Z

Yes, the selector function should be memoized, and when it depends on props, and has to change the component will unsubscribe and subscribe to the source again.

And you are right about the tests, I tried using useSubscription implementation and had to fix the tests to make them pass. Now I think the question is whether to use useSubscription at all, because according to its README it shouldn't be used for Redux/Flux stores. You can check some further discussions regarding this point on the useSubscription PR.

tizoc · 2019-07-22T01:55:17Z

Yeah, I see. There are too many unnecessary notifications because subscriptions are made to the whole state, but most components care only a small part of it.

I'm thinking that the only reason subscriptions make every component get notified is that reads made by selectors (and updates made to the state in reducers) are made through arbitrary code. Functions are opaque, and you don't know what is going on inside.

If updating and reading from the state value was instead done in a more restricted way, such that setters and getters are paired, then it would be possible to track such changes and notify only the components that are actually interested in such change.

[What comes ahead is a very undeveloped idea, and it may be a stupid one, just trying to think about it in a different way. I haven't tried to implement it because a stupid accident I had two nights ago left me able to use only one hand for the next 5 weeks.]

In this example reads and writes are described with values instead of using functions that perform the operations directly. Then functions are used to execute such descriptions and modify the state accordingly.

type state = {
  counter: int,
  content: string,
};

// what comes next can be generated with a ppx rewriter
// see for example https://github.com/Astrocoders/lenses-ppx
// (not sure if I would use that implementation in particular
// but it is good enough for illustration purposes)

type field(_) =
  | Counter: field(int)
  | Content: field(string);

let get: type value. (state, field(value)) => value =
  (state, field) =>
    switch (field) {
    | Counter => state.counter
    // ...
    };

let set: type value. (state, field(value), value) => state =
  (state, field, value) =>
    switch (field) {
    | Counter => {...state, counter: value}
    // ...
    };

let modify: type value. (state, field(value), (value => value)) => state =
  (state, field, update) => {
    let prev = get(state, field);
    let next = update(prev);
    set(state, next);
  };

// TODO: this would have to keep track of what fields are touched
// so that it can be referenced later to decide who to notify
let modifyAndTrack = modify;

let reducer = (state, action) =>
  switch (action) {
  | Increment => modifyAndTrack(state, Counter, add1)
  | Decrement =>  modifyAndTrack(state, Counter, sub1)
  // ...
  };

Components would read values like this:

[@react.component]
let make = () => {
  // useSelector would probably use a combination of useSubscription
  // and useState internally.
  let counter = useSelector(Counter);
  // ...
};

The above example is a huge simplification (and the machinery not implemented), ways to compose selectors are needed too (for example, getting an array field, and then getting index N).

An example using composition and depending on props would be something like this:

[@react.component]
let make = (~userID) => {
  // lets say Users get an assoc list of string -> user
  // from the state value
  let name = useChainedSelectors3(Users, AssocIndex(userID), UserName);

  <div>{name}</div>;
};

Subscriptions would be made to specific selector paths instead of the whole store.

Does this make sense?

tizoc · 2019-07-22T04:02:01Z

Alternative encoding for fields using records that may work better. These include paths and compose easily:

type field('state, 'value) = {
  get: 'state => 'value,
  set: ('value, 'state) => 'state,
  path: string,
};

let getField = (t, field) => field.get(t);
let setField = (value, t, field) => field.set(value, t);
let modifyField = (field, f, t) => field.set(f(field.get(t)), t);
let composeFields = (f1, f2) => {
  let get = t => t |> f1.get |> f2.get;
  let set = value => modifyField(f1, f2.set(value));
  let path = f1.path ++ "/" ++ f2.path;
  {get, set, path};
};

// Example field instance for "counter"
let counter = {
  get: state => state.counter,
  set: (counter, state) => {...state, counter},
  path: "counter",
};

// Array indexing example
let makeArrayIndexField = idx => {
  let path = String.of_int(idx);
  let get = arr => arr[idx];
  let set = (value, arr) => {
    let newArr = Belt.Array.copy(arr);
    newArr[idx] = value;
    newArr;
  };
  { get, set, path };
};

One advantage of this encoding is that supporting custom field updaters and readers is easier, because the path is explicit and the list of fields is not limited to an enum. Things are probably easier on the typing department too when handling subscriptions.

tizoc · 2019-07-26T01:42:21Z

Here is an ugly proof of concept for what I suggested on my previous comments (Index.re is the important part): https://gist.github.com/tizoc/1cae2f1ea466d1e290b04bc35b7c085e

There are a few calls to Js.log in there, uncomment to see how only the relevant notifications get fired.

It is probably not the right direction for reductive, all the fields stuff would grow the library considerably, but in general the approach seems to be viable.

If anyone is interested let me know and I will setup a small runnable demo in a repository (that gist only contains what is relevant for the implementation and is missing all the boilerplate).

Edit:

Better example here
See also: #44 (comment)

…electors

Use useSubscription hook for subscribing to store

MargaretKrutikova · 2019-08-03T17:39:27Z

I have updated the implementation to be based on useSubscription hook, and it does seem much more understandable than the previous one.

The only difference in usage of useSelector is that now every selector function can't be a simple inline function - it needs a stable reference and should be either defined outside of the component or wrapped with useCallback.

Would you be able to check the PR once again, @rickyvetter? Appreciate any further feedback!

nlfiedler · 2019-08-04T01:07:07Z

I've put your latest code into use with JSX 3 components and it's working splendidly. Thank you so much for taking on this important task.

xla · 2019-08-27T13:13:21Z

Hej, just a drive-by bump as this is a great foundation to make reductive usable with hooks. Is there something blocking this PR from going through still, is additional help in the form of testing needed? Happy to lend a hand wherever. /cc @rickyvetter

@MargaretKrutikova Thanks for putting this together, this has been invaluable to us to put together our ReasonReact application.

MargaretKrutikova · 2019-09-02T21:21:30Z

Since the official use-subscription hook has made its way into npm, I have updated the PR to use the official package with reason bindings. Now it is even less code in the useSelector, and the tests are green 🙂

rickyvetter · 2019-09-03T15:39:24Z

<3 thanks for all of your work on this and apologies for being crazy slow on the response here. This looks great!

MargaretKrutikova added 8 commits July 13, 2019 12:43

Update reason-react and bs-platform, add hooks

b57ddac

Update examples to reductive hooks

b361b37

Convert todo and render examples to hooks

410db21

Remove package-specs from bs-config

03c39ee

Remove some comments

48ec304

Revert back support for old jsx syntax

0884d88

Remove namespace

a6c5989

Simplify creating context

326653e

Add tests for useSelector

d4eb8c6

rickyvetter requested changes Jul 15, 2019

View reviewed changes

MargaretKrutikova mentioned this pull request Jul 17, 2019

useSubscription hook facebook/react#15022

Merged

tizoc mentioned this pull request Jul 27, 2019

Support for JSX 3 and [@react.component] #44

Closed

MargaretKrutikova added 4 commits July 28, 2019 22:38

Use useSubscription hook in useSelector, fix tests to have memoized s…

cd0b6a2

…electors

Check for updates after subscribing

bb9279e

Remove generated js from gitignore

1db2268

Fix examples for selector with subscription

2abdf80

MargaretKrutikova and others added 2 commits August 3, 2019 18:54

Add generated JS, make interface type more descriptive

5232881

Merge pull request #1 from MargaretKrutikova/hooks-subscription

dd9569b

Use useSubscription hook for subscribing to store

Use official useSubscription bindings

0c2f20a

rickyvetter merged commit 53e5836 into rickyvetter:master Sep 3, 2019

tizoc mentioned this pull request Sep 3, 2019

Make reason-react into a peer dependency. #46

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for jsx3 with hooks #45

Support for jsx3 with hooks #45

MargaretKrutikova commented Jul 13, 2019

nlfiedler commented Jul 13, 2019

MargaretKrutikova commented Jul 13, 2019

MargaretKrutikova commented Jul 14, 2019

rickyvetter left a comment

rickyvetter Jul 15, 2019

MargaretKrutikova Aug 3, 2019

rickyvetter Jul 15, 2019 •

edited

rickyvetter Jul 15, 2019

MargaretKrutikova Aug 3, 2019

rickyvetter Jul 15, 2019

MargaretKrutikova Aug 3, 2019

rickyvetter Jul 15, 2019

rickyvetter Jul 15, 2019

rickyvetter Jul 15, 2019

rickyvetter Jul 15, 2019

rickyvetter commented Jul 15, 2019

rickyvetter commented Jul 15, 2019

MargaretKrutikova commented Jul 15, 2019

rickyvetter commented Jul 16, 2019

MargaretKrutikova commented Jul 16, 2019

tizoc commented Jul 17, 2019 •

edited

tizoc commented Jul 19, 2019

MargaretKrutikova commented Jul 19, 2019

tizoc commented Jul 22, 2019

tizoc commented Jul 22, 2019 •

edited

tizoc commented Jul 26, 2019 •

edited

MargaretKrutikova commented Aug 3, 2019

nlfiedler commented Aug 4, 2019

xla commented Aug 27, 2019

MargaretKrutikova commented Sep 2, 2019

rickyvetter commented Sep 3, 2019

Support for jsx3 with hooks #45

Support for jsx3 with hooks #45

Conversation

MargaretKrutikova commented Jul 13, 2019

nlfiedler commented Jul 13, 2019

MargaretKrutikova commented Jul 13, 2019

MargaretKrutikova commented Jul 14, 2019

rickyvetter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickyvetter Jul 15, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickyvetter commented Jul 15, 2019

rickyvetter commented Jul 15, 2019

MargaretKrutikova commented Jul 15, 2019

rickyvetter commented Jul 16, 2019

MargaretKrutikova commented Jul 16, 2019

tizoc commented Jul 17, 2019 • edited

tizoc commented Jul 19, 2019

MargaretKrutikova commented Jul 19, 2019

tizoc commented Jul 22, 2019

tizoc commented Jul 22, 2019 • edited

tizoc commented Jul 26, 2019 • edited

MargaretKrutikova commented Aug 3, 2019

nlfiedler commented Aug 4, 2019

xla commented Aug 27, 2019

MargaretKrutikova commented Sep 2, 2019

rickyvetter commented Sep 3, 2019

rickyvetter Jul 15, 2019 •

edited

tizoc commented Jul 17, 2019 •

edited

tizoc commented Jul 22, 2019 •

edited

tizoc commented Jul 26, 2019 •

edited