Allow Immutable.List (or Iterable) to be passed

[cristian-sima]

Let's take this code:

const notifications = Immutable.List();

...

return <NotificationStack
      notifications={notifications}
      ...
    />);

The code works, but in the console I have this error:

app.js:9323 Warning: Failed prop type: Invalid prop `notifications` of type `object` supplied to `NotificationStack`, expected `array`.
    in NotificationStack (created by SnackBar)
    in SnackBar (created by Connect(SnackBar))
    in Connect(SnackBar) (created by Sidebar)
    in div (created by Sidebar)
    in Sidebar (created by Connect(Sidebar))
    in Connect(Sidebar) (created by CompanyPage)
    in CompanyPage (created by Connect(CompanyPage))
    in Connect(CompanyPage) (created by RouterContext)
    in RouterContext (created by Router)
    in Router (created by Root)
    in Provider (created by Root)
    in Root
    in AppContainer

I will lose the benefits of Immutable if I change the code to Immutable.List().toArray (or toJS)) as presented here

Also here.

Other comment:

Good question. I Wouldn't do it in the store though, since then you lose the abillity to do a simple object comparison (prevState !== this.state) if you'd want to optimize rendering with shouldComponentUpdate

[pburtchaell]

Looks like what we need to do here is update the value of NotificationStack.propTypes. notifications to allow any Iterable, not just an Array. I'm fine with this.

Do you know what the value should be changed to? I don't have any experience adding validation for Immutable objects. It looks like react-immutable-proptypes exists, but I don't want to add a dependency. Can we solve this problem otherwise?

[cristian-sima]

Yep.

import {List} from "immutable";

...

proptypes = {
...
notifications: instanceOf(List)
}

[cristian-sima]

Or just do not check the notifications. I think this is the best way

[BerkeleyTrue]

@cristian-sima Does react except accepts an immutable lists of elements in place of an array of elements?

[cristian-sima]

@BerkeleyTrue I think you've intended to ask if it accepts. Yes. It accepts, as the Immutable.List has the interface of a Iterable (which must contain a .map, a .filter and so on)

[cristian-sima]

@BerkeleyTrue

But in either case you want to do it, I will appreciate very much if you want to solve it.

It appears all day as a strange warning and I can not turn it off

[pburtchaell]

just do not check the notifications. I think this is the best way

The component should always check props. It's useful when debugging.

import {List} from "immutable";

I don't want to add any dependencies.

The only solution would be to add custom validation based off code from react-immutable-proptypes. However, this is a micro-optimization and I don't have time to add this now. If you'd like a take a stab at it @cristian-sima, that would be great.

[BerkeleyTrue]

Looking through the cod here https://github.com/facebook/immutable-js/blob/master/src/Iterable.js#L39

It doesn't look like that propType check would work outside of Immutable types so we would end up with warnings for everything other than Immutable Iterable.

[pburtchaell]

It's possible to write a custom prop validation that checks for either an Array or an Iterable. Look for the customProp example here: https://facebook.github.io/react/docs/reusable-components.html.

[joeybaker]

react-immutable-prototypes solves this nicely.

[BerkeleyTrue]

@joeybaker As mentioned above, that only checks for immutable iterables and not iterables in general

https://github.com/HurricaneJames/react-immutable-proptypes/blob/master/src/ImmutablePropTypes.js#L32

It would also mean everyone would be pulling in (bundling) react-immutable-proptypes as well as immutable whether they use it or not.

[pburtchaell]

Correct, @joeybaker, but I'd like to keep the project dependency free and as @BerkeleyTrue mentioned, the check from that library is for an immutable Iterable only. To resolve this issue, we need to adapt the code from that library to be a custom prop validation that checks for both an immutable Iterable and an Array.

[BerkeleyTrue]

Could it be this simple?

function isMappable(props, propName) {
  const maybeMappable = props[propName];
  return !!(maybeMappable && Array.isArray(maybeMappable) && typeof maybeMappable.map === 'function');
}

Added array check

[pburtchaell]

I think the first step of this would employ the use of Array.isArray. If that condition fails, then test if the prop an immutable Iterable. I'm not sure what the best condition for the Iterable would be. If map works, then let's use that.

[BerkeleyTrue]

Yes, checking for map would prevent strings, which are iterable, from passing validation.

[cristian-sima]

Is it working in this way?

[cristian-sima]

@pburtchaell @BerkeleyTrue ?

[pburtchaell]

I'm not available in terms of time to implement this feature. If you need it now, please submit a pull request!

On Oct 6, 2016 08:31, "Cristian Sima" notifications@github.com wrote:

@pburtchaell https://github.com/pburtchaell @BerkeleyTrue https://github.com/BerkeleyTrue ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pburtchaell/react-notification/issues/92#issuecomment-251962096, or mute the thread https://github.com/notifications/unsubscribe-auth/ADrbrGY4vNRRDfnP3IJwjmkSuqW4Dqcdks5qxPg7gaJpZM4J82-w .

[pburtchaell]

I'm closing because this is a micro-optimization. If you have time to implement a dependency free prop check, @cristian-sima, please create a PR.

[cristian-sima]

@pburtchaell @BerkeleyTrue I think there is no simple way of checking if that is an Iterable in a free dependency way. However, I am thinking of another way of approaching this problem.

We can create a new branch called support-immutablejs which contains this dependency react-immutable-proptypes (and implicity the immutable dependecy). Taking into account that those interested in using support-immutablejs will already use immutable, they will not find it as a problem.

For all others, there will be the master branch which is free dependent. For any updates on master, we can update the support-immutablejs branch from master.

What do you think?

[pburtchaell]

Hey, @cristian-sima. I think this would work, but I would rather not maintain official support for Immutable. If you would like to maintain a Immutable supported fork of the project for your own purposes, please do.