Introduce Lens to allow for conditional updates #35

nlfiedler · 2018-09-07T04:13:43Z

The Lens is used to modify the state into the shape that the component
requires, which allows Reductive.Provider to implement shouldUpdate(). This
avoids needless updates when another part of the application state changes.
You can observe the change using React devtools "highlight updates."

Introduces a new type, Lens, and all users of Provider must now provide an
implementation of this type as an argument to createMake(). Wanted to make
this argument optional, but the default would return a 'state instead of a
'lensed, and I didn't know how to resolve that.

This change includes a new example specifically constructed to demonstrate
the problem, or lack thereof now that Lens is in use by Provider.

The Lens is used to modify the state into the shape that the component requires, which allows Reductive.Provider to implement shouldUpdate(). This avoids needless updates when another part of the application state changes. You can observe the change using React devtools "highlight updates." Introduces a new type, Lens, and all users of Provider must now provide an implementation of this type as an argument to createMake(). Wanted to make this argument optional, but the default would return a 'state instead of a 'lensed, and I didn't know how to resolve that. This change includes a new example specifically constructed to demonstrate the problem, or lack thereof now that Lens is in use by Provider.

rickyvetter

I think the changes are pretty aggressive. Is it possible to get an API that isn't breaking?

rickyvetter · 2018-11-10T22:20:31Z

src/reductive.re

@@ -49,40 +55,40 @@ module Provider = {
  type action =
  | UpdateState
  | AddListener(action => unit);
-  let createMake = (~name="Provider", store: Store.t('action, 'state)) => {
+  let createMake = (~name="Provider", store: Store.t('action, 'state), lens: Lens.t('state, 'lensed)) => {


This is a pretty major breaking change. Why not make this an optional argument that defaults to identity? I think this would provide the same behavior as an option for more complex cases without ruining existing code.

rickyvetter · 2018-11-10T22:22:02Z

src/reductive.re

+module Lens = {
+  type t('a, 'b) = 'a => 'b;
+  let make = getter => getter;
+  let view = (l, a) => l(a);


Lenses typically have update functions as well. I think this would probably be helpful for writing large reducers. Not blocking this PR of course, but throwing it out there as something to look into.

rickyvetter · 2018-11-11T00:59:50Z

I see your comment on #27 and then was reading up on this issue and I now see why you're running into trouble with this. Let me think about this a bit more.

https://stackoverflow.com/questions/48363986/force-a-broader-type-for-optional-argument-with-more-restrictive-default-value

rickyvetter · 2018-11-16T18:07:51Z

I think lenses might fit better as a separate dedicated component, or maybe even as a hook in the new React api. Have you looked into making it as a separate component?

nlfiedler · 2018-11-18T01:29:34Z

I gave this a good deal of thought and could not manage to make this change in a way that satisfied me. It works, it's a bunch of wretched duplicate code, but it works, and the lens is now optional. I tried making an optional "getState" function, but ran into the same problem as before, with trying to make the lens optional. I also wondered if there could have been a thin provider that would be wrapped by the plain Provider, but that seemed like it would be difficult to get away with, and likely end up being just as much code. I'm open to any ideas that will make this better.

About the lens having an update function, that could be neat, but I would prefer to handle that separately. Thanks.

rickyvetter · 2018-11-18T20:17:18Z

So it seems like a lot of this duplication is a result of shoe-horning a solution into an existing API that Reason has some trouble typing. I think there are a few ways we can solve this in a completely different way that plays nicely with everything involved. How would you feel about a function like this:

let lensedComponent = (make, get, ~state, ~dispatch, children) => {
  let state = get(state);
  let baseComponent = make(~state, ~dispatch, children);
  {
    ...baseComponent,
    ReasonReact.retainedProps: state,
    shouldUpdate:
      (
        {
          oldSelf: {retainedProps: oldState},
          newSelf: {retainedProps: newState},
        } as total,
      ) =>
      oldState === newState ?
        false : baseComponent.ReasonReact.shouldUpdate(total),
  };
};

The pros here: no code duplication, no unintuitive name change to get lensed behavior, no breaking change, minimal api addition.

The cons: the api kinda looks like it'll work for any component, but actually needs the exact reductive component api for the make function it wraps. It does have a constant-time overhead on each call because it's spreading the baseComponent record.

There are a few other ways to do this as well, but this is the one I can think of now that has my personal favorite set of tradeoffs. Interested to hear if it'll work for your use case and how you feel about it.

NOTE: didn't actually run this code. Just wrote it up and typechecked it - but I think it should work.

nlfiedler · 2018-11-19T17:07:05Z

Will that operate in the same manner as the lensed provider does now? Also, could I get your help with the type signatures? I can't seem to get it right.

let lensedComponent =
    (
      make:
        ('lensed, 'action => unit, array(ReasonReact.reactElement)) =>
        ReasonReact.component('a, 'b, 'c),
      get: 'state => 'lensed,
      ~state: 'state,
      ~dispatch: 'action => unit,
      children: array(ReasonReact.reactElement),
    )
    : ReasonReact.component('a, 'b, 'c) => {
  let state: 'lensed = get(state);
  let baseComponent = make(~state, ~dispatch, children);
  /* ... */
};

The ~state in the last line has an error:

  119 ┆   : ReasonReact.component('a, 'b, 'c) => {
  120 ┆ let state: 'lensed = get(state);
  121 ┆ let baseComponent = make(~state, ~dispatch, children);
  122 ┆ {
  123 ┆   ...baseComponent,

  The function applied to this argument has type
    ('action => unit, array(ReasonReact.reactElement)) =>
    ReasonReact.componentSpec('a, 'a, 'b, 'b, 'c)
This argument cannot be applied with label ~state

rickyvetter

Will that operate in the same manner as the lensed provider does now?

It's a different api but it should have the same effect.

could I get your help with the type signatures

It should be make: (~state: 'lensed, ~dispatch: 'action => unit, array(ReasonReact.reactElement)). You need the named args.

rickyvetter · 2018-11-19T17:12:20Z

examples/render/renderEntry.re

+
+module CounterProvider = {
+  let lens = Reductive.Lens.make(a => a.counter);
+  let make = Reductive.LensedProvider.createMake(store, lens);


With my proposal this would just be a Provider - no need for a LensedProvider module...

rickyvetter · 2018-11-19T17:13:28Z

examples/render/renderEntry.re

+    ...component,
+    render: _self =>
+      <div>
+        <CounterProvider component=CounterComponent.make />


... and this would look like:

Suggested change

<CounterProvider component=CounterComponent.make />

<CounterProvider component=Reductive.lensedComponent(CounterComponent.make, s => s.counter) />

nlfiedler · 2018-11-20T01:55:53Z

Yeah, I tried that and got Error: Syntax error: Labels are not allowed inside a tuple not expected. I'll keep playing with it.

rickyvetter · 2018-11-20T20:17:08Z

There's an issue with refmt - you can write this way, but refmt here is not stable.

let lensedComponent =
    (
      make:
        ((
          ~state: 'lensed,
          ~dispatch: 'action => unit,
          array(ReasonReact.reactElement)
        ) =>
        ReasonReact.component('a, 'b, 'c)),
      get: 'state => 'lensed,
      ~state: 'state,
      ~dispatch: 'action => unit,
      children: array(ReasonReact.reactElement),
    )
    : ReasonReact.component('a, 'b, 'c) => {
  let state: 'lensed = get(state);
  let baseComponent = make(~state, ~dispatch, children);
  {
    ...baseComponent,
    ReasonReact.retainedProps: state,
    shouldUpdate:
      (
        {
          oldSelf: {retainedProps: oldState},
          newSelf: {retainedProps: newState},
        } as total,
      ) =>
      oldState === newState ?
        false : baseComponent.ReasonReact.shouldUpdate(total),
  };
};

reasonml/reason#2271

nlfiedler · 2018-11-21T04:05:42Z

Terrific, thanks for tracking down that issue with the parsing, good to know it's fixed. I've adjusted the types in the function signature to get things compiling, and had to change the components in question to ...WithRetainedProps and add the retainedProps field. So far so good, but the React "highlight updates" shows that the entire application is rendering if any change is made. I've pushed the latest code for comparison. I'll continue to look at this...

ambientlight · 2018-11-25T19:49:24Z

@nlfiedler: in your latest code the lense is wrapped inside the provider, which gets re-rendered on every state change since its internal state type state('reductiveState) = { reductiveState: option('reductiveState), unsubscribe: option(unit => unit)}; changes on every update.

@rickyvetter: Why does the lense fits better as separate component. Isn't it just a provider that narrows down the state?

ambientlight · 2018-11-25T21:34:56Z

What about this kind of provider. Doesn't break existing functionality.

module Provider = {
  type state('reductiveState) = {
    reductiveState: option('reductiveState),
    unsubscribe: option(unit => unit)
  };

  type action =
    | UpdateState
    | AddListener(action => unit);
  
  let createLenseMake = (~name="Lense", ~lense:'state => 'lense, store: Store.t('action, 'state)) => {
    let innerComponent = ReasonReact.reducerComponent(name);
    
    let make = (~component: (~state: 'lense, ~dispatch: 'action => unit, array(ReasonReact.reactElement)) => ReasonReact.component('a, 'b, 'c), _children: array(ReasonReact.reactElement)): 
    ReasonReact.component(state('lense), ReasonReact.noRetainedProps, action) => {
      ...innerComponent,
      
      initialState: () => {
        reductiveState: Some(lense(Store.getState(store))),
        unsubscribe: None
      },

      reducer: (action, state) =>
        switch (action) {
        | AddListener(send) =>
          ReasonReact.Update({
            unsubscribe: Some(Store.subscribe(store, (_) => send(UpdateState))),
            reductiveState: Some(lense(Store.getState(store)))
          })
        | UpdateState =>
          ReasonReact.Update({
            ...state,
            reductiveState: Some(lense(Store.getState(store)))
          })
      },
      
      didMount: ({send}) => send(AddListener(send)),

      willUnmount: ({state}) =>
        switch (state.unsubscribe) {
        | Some(unsubscribe) => unsubscribe()
        | None => ()
        },

      shouldUpdate: ({oldSelf, newSelf}) => {
        oldSelf.state.reductiveState !== newSelf.state.reductiveState;
      },

      render: ({state}) =>
        switch (state.reductiveState) {
        | None => ReasonReact.null
        | Some(state) =>
          ReasonReact.element(
            component(~state, ~dispatch=Store.dispatch(store), [||])
          )
        }
    };
    
    make;
  };

  let createMake = (~name="Provider", store: Store.t('action, 'state)) => createLenseMake(~name, ~lense=s => s, store)
};

let make = Reductive.Provider.createMake(store);
let make = Reductive.Provider.createLenseMake(~lense=store=>store.content, store);

This is somehow similar to what was originally proposed here, though I though referential comparison should be used, comparing complex substrate structurally might get expensive.

nlfiedler · 2018-11-26T17:18:06Z

@ambientlight That was awesome, thank you. A simple solution that resolves all of the issues, I believe. The code now works as intended, rendering only the components that actually changed, and is backward compatible. Thanks for the code and suggestion.

@rickyvetter Please let me know what you think of this latest code. Also, I'm happy to squash these commits, if so desired. The work in progress is kinda of interesting, but also messy.

rickyvetter

Yeah I think this implementation is pretty good. I originally wanted the lensing feature to be completely separate because you might be able to use many different lenses with a single subscriber. I'm not actually confident that would work though and I'm happy with adding this api.

rickyvetter · 2018-11-26T17:30:36Z

src/reductive.re

@@ -92,6 +97,8 @@ module Provider = {
        | Some(unsubscribe) => unsubscribe()
        | None => ()
        },
+      shouldUpdate: ({oldSelf, newSelf}) =>
+        oldSelf.state.reductiveState != newSelf.state.reductiveState,


As @ambientlight recommended this should really be referential comparison. This will run a quite expensive comparison on deeply nested data structures.

Suggested change

oldSelf.state.reductiveState != newSelf.state.reductiveState,

oldSelf.state.reductiveState !== newSelf.state.reductiveState,

rickyvetter · 2018-11-26T17:36:44Z

Yeah if you could squash the wip commits that would be great!

ambientlight · 2018-11-26T17:43:01Z

@rickyvetter, @nlfiedler: I was thinking maybe separate components would actually look more neat.
Let me quickly throw an implementation (with no code duplication)

ambientlight · 2018-11-26T18:49:24Z

@rickyvetter, @nlfiedler: How about this?

module Lense = {
  type state('reductiveState) = {
    reductiveState: option('reductiveState),
    unsubscribe: option(unit => unit)
  };

  type action =
    | UpdateState
    | AddListener(action => unit);
  
  let createMake = (~name="Lense", ~lense:'state => 'lense, store: Store.t('action, 'state)) => {
    let innerComponent = ReasonReact.reducerComponent(name);
    
    let make = (~component: (~state: 'lense, ~dispatch: 'action => unit, array(ReasonReact.reactElement)) => ReasonReact.component('a, 'b, 'c), _children: array(ReasonReact.reactElement)): 
    ReasonReact.component(state('lense), ReasonReact.noRetainedProps, action) => {
      ...innerComponent,
      
      initialState: () => {
        reductiveState: Some(lense(Store.getState(store))),
        unsubscribe: None
      },

      reducer: (action, state) =>
        switch (action) {
        | AddListener(send) =>
          ReasonReact.Update({
            unsubscribe: Some(Store.subscribe(store, (_) => send(UpdateState))),
            reductiveState: Some(lense(Store.getState(store)))
          })
        | UpdateState =>
          ReasonReact.Update({
            ...state,
            reductiveState: Some(lense(Store.getState(store)))
          })
      },
      
      didMount: ({send}) => send(AddListener(send)),

      willUnmount: ({state}) =>
        switch (state.unsubscribe) {
        | Some(unsubscribe) => unsubscribe()
        | None => ()
        },

      shouldUpdate: ({oldSelf, newSelf}) => 
        oldSelf.state.reductiveState !== newSelf.state.reductiveState
      ,

      render: ({state}) =>
        switch (state.reductiveState) {
        | None => ReasonReact.null
        | Some(state) =>
          ReasonReact.element(
            component(~state, ~dispatch=Store.dispatch(store), [||])
          )
        }
    };
    
    make;
  };
};

module Provider {
  type state('reductiveState) = Lense.state('reductiveState);
  type action = Lense.action;
  let createMake = (~name="Provider", store: Store.t('action, 'state)) => Lense.createMake(~name, ~lense=s => s, store)
};

rickyvetter · 2018-11-26T18:51:13Z

Yeah I have a preference for the separate modules definitely.

Using code from Taras Vozniuk, the Provider component now delegates to a new Lense component, which contains the bulk of the original Provider code. The "new" Provider implements a default identity function for the lense. This approach retains the desired behavior of rendering only the changed parts of the application, as well as remaining backward compatible. Seems that refmt changed some parentheses to curly braces.

nlfiedler · 2018-11-27T02:40:44Z

@rickyvetter I squashed the recent changes, up to the point of the merge from master.

@ambientlight Thanks for the code, this works splendidly.

rickyvetter · 2018-11-27T04:40:46Z

Looks good. Thanks all. And thanks for making the extra effort to make this a clean and non-breaking addition.

This was referenced Sep 7, 2018

Reductive doesn't avoid supurious rerenders #27

Open

Usage discouragement in readme #26

Open

wyze requested a review from rickyvetter November 8, 2018 12:50

rickyvetter requested changes Nov 10, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into render

39755db

rickyvetter reviewed Nov 19, 2018

View reviewed changes

rickyvetter requested changes Nov 26, 2018

View reviewed changes

rickyvetter merged commit d9903eb into rickyvetter:master Nov 27, 2018

nlfiedler deleted the render branch November 29, 2018 02:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Lens to allow for conditional updates #35

Introduce Lens to allow for conditional updates #35

nlfiedler commented Sep 7, 2018

rickyvetter left a comment

rickyvetter Nov 10, 2018

rickyvetter Nov 10, 2018

rickyvetter commented Nov 11, 2018

rickyvetter commented Nov 16, 2018

nlfiedler commented Nov 18, 2018 •

edited

Loading

rickyvetter commented Nov 18, 2018 •

edited

Loading

nlfiedler commented Nov 19, 2018

rickyvetter left a comment

rickyvetter Nov 19, 2018

rickyvetter Nov 19, 2018

nlfiedler commented Nov 20, 2018

rickyvetter commented Nov 20, 2018 •

edited

Loading

nlfiedler commented Nov 21, 2018

ambientlight commented Nov 25, 2018

ambientlight commented Nov 25, 2018 •

edited

Loading

nlfiedler commented Nov 26, 2018

rickyvetter left a comment

rickyvetter Nov 26, 2018

rickyvetter commented Nov 26, 2018

ambientlight commented Nov 26, 2018 •

edited

Loading

ambientlight commented Nov 26, 2018

rickyvetter commented Nov 26, 2018

nlfiedler commented Nov 27, 2018

rickyvetter commented Nov 27, 2018

	<CounterProvider component=CounterComponent.make />
	<CounterProvider component=Reductive.lensedComponent(CounterComponent.make, s => s.counter) />

	oldSelf.state.reductiveState != newSelf.state.reductiveState,
	oldSelf.state.reductiveState !== newSelf.state.reductiveState,

Introduce Lens to allow for conditional updates #35

Introduce Lens to allow for conditional updates #35

Conversation

nlfiedler commented Sep 7, 2018

rickyvetter left a comment

Choose a reason for hiding this comment

rickyvetter Nov 10, 2018

Choose a reason for hiding this comment

rickyvetter Nov 10, 2018

Choose a reason for hiding this comment

rickyvetter commented Nov 11, 2018

rickyvetter commented Nov 16, 2018

nlfiedler commented Nov 18, 2018 • edited Loading

rickyvetter commented Nov 18, 2018 • edited Loading

nlfiedler commented Nov 19, 2018

rickyvetter left a comment

Choose a reason for hiding this comment

rickyvetter Nov 19, 2018

Choose a reason for hiding this comment

rickyvetter Nov 19, 2018

Choose a reason for hiding this comment

nlfiedler commented Nov 20, 2018

rickyvetter commented Nov 20, 2018 • edited Loading

nlfiedler commented Nov 21, 2018

ambientlight commented Nov 25, 2018

ambientlight commented Nov 25, 2018 • edited Loading

nlfiedler commented Nov 26, 2018

rickyvetter left a comment

Choose a reason for hiding this comment

rickyvetter Nov 26, 2018

Choose a reason for hiding this comment

rickyvetter commented Nov 26, 2018

ambientlight commented Nov 26, 2018 • edited Loading

ambientlight commented Nov 26, 2018

rickyvetter commented Nov 26, 2018

nlfiedler commented Nov 27, 2018

rickyvetter commented Nov 27, 2018

nlfiedler commented Nov 18, 2018 •

edited

Loading

rickyvetter commented Nov 18, 2018 •

edited

Loading

rickyvetter commented Nov 20, 2018 •

edited

Loading

ambientlight commented Nov 25, 2018 •

edited

Loading

ambientlight commented Nov 26, 2018 •

edited

Loading