Doesn't correctly support multiple props

[eric-burel]

Description

If you have 2 immutable props A and B, a change in A will also recompute .toJS() for B.

Steps to reproduce

Sharing the failing test and the fix:

// doesn't pass! B is recomputed but should not
import React, { useState } from 'react';
import { render, fireEvent, screen } from '@testing-library/react';
import { fromJS } from 'immutable';
mport withImmutablePropsToJS from 'with-immutable-props-to-js';

describe('withImmutablePropsToJS', () => {
  test('should have two renders when state is updated', async () => {
    const Counter = ({ A, B }) => {
      const [initA] = useState(A);
      const [initB] = useState(B);
      return (
        <div>
          <p>
            {initA === A ? 'A is same' : 'A is not same'}
            {initB === B ? 'B is same' : 'B is not same'}
          </p>
        </div>
      );
    };
    const WithImmutableCounter = withImmutablePropsToJS(Counter);
    const Component = () => {
      const [immutA, setImmutA] = useState(fromJS({}));
      const [immutB /* , setImmutB */] = useState(fromJS({}));
      return (
        <div>
          <button
            type="button"
            onClick={() => {
              console.log('setting immutA');
              setImmutA(fromJS({ foo: 'bar' }));
            }}
          >
            Trigger update A
          </button>
          <WithImmutableCounter A={immutA} B={immutB} />
        </div>
      );
    };
    // const { renderCount } = perf(React);

    render(<Component />);

    fireEvent.click(screen.getByRole('button', { name: /Trigger update A/i }));
    const testParagraph = document.querySelector('p');
    expect(testParagraph).toHaveTextContent(/A is not same/);
    expect(testParagraph).toHaveTextContent(/B is same/);
    // await wait(() => expect(renderCount.current.Counter.value).toBe(2));
  });
});

The fix:

Fix (it's a naive 5 minute work so be careful! it might not work as expected!):
Also since it introduces memoization, there's a tradeoff, it's overkill if you don't have any performance issue.
It could maybe be exposed as another hoc in the same package, like "withMemoizedImmutablePropsToJS"
Edit: fixing initial computation, so state is initialized with the full values

// @see https://github.com/tophat/with-immutable-props-to-js
// @see https://github.com/tophat/with-immutable-props-to-js/issues/63
// this is a fix for this library that correctly handles multiple props

/**
 * Original work Copyright (c) 2015-2018 Dan Abramov
 *
 * Permission is hereby granted, free of charge, to any person obtaining a copy
 * of this software and associated documentation files (the "Software"), to
 * deal in the Software without restriction, including without limitation the
 * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
 * sell copies of the Software, and to permit persons to whom the Software is
 * furnished to do so, subject to the following conditions:
 *
 * The above copyright notice and this permission notice shall be included in
 * all copies or substantial portions of the Software.
 *
 *
 * Modified work Copyright 2018-present Top Hat Monocle Corp.
 *
 * https://github.com/tophat/with-immutable-props-to-js/blob/master/LICENSE
 */

import hoistNonReactStatics from 'hoist-non-react-statics';
import React, { useState, useRef } from 'react';
import { Iterable } from 'immutable';
import PropTypes from 'prop-types';

const getDisplayName = Component => {
  return Component.displayName || Component.name || 'Component';
};

const withImmutablePropsToJS = WrappedComponent => {
  const Wrapper = props => {
    const { forwardedRef, ...rest } = props;
    const previousImmutableProps = useRef({});
    const nextPropsJS = Object.entries(rest).reduce(
      (newProps, [propKey, propValue]) => {
        // it's an immutable object
        const canConvertToJS =
          Iterable.isIterable(propValue) &&
          typeof propValue.toJS === 'function';
        // console.log('converting', propKey, propValue, canConvertToJS);
        // it has actually changed
        if (previousImmutableProps.current[propKey] !== propValue) {
          // remember this value as the last known props
          previousImmutableProps.current[propKey] = propValue;
          // rerun toJS()
          newProps[propKey] = canConvertToJS ? propValue.toJS() : propValue;
        }
        return newProps;
      },
      {}
    );
    const [propsJS, setPropsJS] = useState(nextPropsJS); // init with the current props

    // update state if needed
    if (Object.keys(nextPropsJS).length > 0) {
      setPropsJS(currentProps => ({ ...currentProps, ...nextPropsJS }));
    }
    return <WrappedComponent {...propsJS} ref={forwardedRef} />;
  };

  Wrapper.propTypes = {
    forwardedRef: PropTypes.oneOfType([
      PropTypes.func,
      PropTypes.shape({ current: PropTypes.node })
    ])
  };

  Wrapper.defaultProps = {
    forwardedRef: null
  };

  const WrapperWithForwardedRef = React.forwardRef((props, ref) => (
    <Wrapper {...props} forwardedRef={ref} />
  ));

  WrapperWithForwardedRef.displayName = `withImmutablePropsToJS(${getDisplayName(
    WrappedComponent
  )})`;
  WrapperWithForwardedRef.WrappedComponent = WrappedComponent;

  hoistNonReactStatics(WrapperWithForwardedRef, WrappedComponent);

  return WrapperWithForwardedRef;
};

export default withImmutablePropsToJS;

[msrose]

Hi Eric,

Thanks for providing a detailed example.

What's the use case here for requiring prop conversion to be done independently? Is it possible to change how your components are connected to Redux to achieve the same result?

e.g. instead of

const Child1 = ({ a }) => { /*...*/ }
const Child2 = ({ b }) => { /*...*/ }

const Parent = (a, b) => {
  <>
    <Child1 a={a} />
    <Child2 b={b} />
  </>
};

// ...

export default connect(mapStateToProps)(withImmutablePropsToJs(Parent));

the following should achieve the desired result because of the built-in behaviour of connect (or useSelector for that matter) to save rerenders.

const Child1 = connect(mapStateToPropsA)(withImmutablePropsToJs(({ a }) => { /*...*/ }))
const Child2 = connect(mapStateToPropsB)(withImmutablePropsToJs(({ b }) => { /*...*/ }))

const Parent = () => {
  <>
    <Child1 />
    <Child2 />
  </>
};

// ...

export default Parent;

I have generally assumed re-render saving to be out of scope of this library because it already gets handled by react-redux, and as you mentioned it just increases memory footprint and adds complexity that is often an unnecessary optimization.

[msrose]

Another option may be to preempt .toJS conversion for certain props that are causing you problems, e.g.

const Child1 = ({ a }) => { /*...*/ }
const Child2 = ({ b }) => { /*...*/ }

const Parent = withImmutablePropsToJs((a, b) => {
  <>
    <Child1 a={a} />
    <Child2 b={b} />
  </>
});

// ...

const ParentWrapper = ({ a, b }) => {
  const aToJs = useMemo(() => a.toJS(), [a])
  <Parent a={aToJs} b={b} />
};

export default connect(mapStateToProps)(ParentWrapper);

This is close to what you suggested above and might make sense as a supplemental component or separate API of this library.

[eric-burel]

The use case is a big nested object living in the state:

{ project: { widgets: [...], pages: [...], metaData: {...}, layouts: [...]} // and the list goes on

and so on. The problem is that most component will subscribe to the project info + other underlying data such as the widgets or pages. Each container connects to a lot of interrelated data (around 10 props). So it's common that project change because say metaData has changed, but not pages => component that only use pages should not rerender just because the parent is generating a new JS object. You can connect them but that can become annoying when you have a lot of props.

It's true that my state is maybe ill-designed and not flat enough, also I indeed need to connect more child components to the Redux state. This code dates from before the hook period so I lacked easy memoization in particular, and where redux was all the rage so we used it a lot to cache model objects that came from the backend as is.

Yet I feel more confident in Immutable now that I am sure that all my JS object are guaranteed to change reference only when they change value, as the default behaviour, hence my initial proposal.

To summarize my idea: Immutable, as used in most apps, is breaking Redux principle of immutability, not because it mutates values, but because it creates new object where there should be no new object, because of the mandatory final call to .toJS. So I try to put it where it belongs: managing immutability in reducers, easy data querying in selectors, and nothing more. Which implies doing the .toJS conversion only if strictly necessary.

Thanks for the alternatives you propose, they are interesting!

[msrose]

Nice okay that gives me a better idea of what you're working with, thanks. I'm going to mull this over a bit and it might make sense to add some APIs to help with your use case, since the official word is to use Immer with Redux now. So this library might be best suited to pivoting towards helping people migrate away from Immutable.js. Hopefully those alternatives help unblock you in the meantime.

[eric-burel]

Yeah definitely, I think a link to this ticket in the Readme would probably be sufficient, given that only legacy apps are concerned.
I've updated the code with a small fix on initialization.