gor181 / react-notification-system-redux

Redux wrapper for react-notification-system
MIT License
324 stars 59 forks source link

Immutable support #14

Open zeevl opened 8 years ago

zeevl commented 8 years ago

This adds support for immutable stores.

It does so by making the notification store immutable itself. This works if store.notifications is handled abstractly by the consumer as documented, even if the consumer itself isn't using immutable elsewhere (as in the example).

In other words, this change allows react-notification-system-redux to be compatible with both immutable and plain old object stores, whereas previously it was only compatible with plain object stores.

Of course, this approach will break anyone who is using store.notifications outside of react-notification-system-redux, since it changes the structure of said object. Not sure how much of a concern that is though.

gor181 commented 8 years ago

hey @zeevl ,

Thanks for the PR, I really appreciate it! What happens if "pojo's" are passed as notifications?

I guess it will still be an array? Will the code throw? What I want to ask is, will this changes break existing applications which are using this lib?

zeevl commented 8 years ago

By notifications are you referring to the objects passed to Notifications.success() etc? Those are still expected to be the same POJOs; the api hasn't changed around that.

The only place I can think this could break existing applications is if they're accessing the store.notifications array directly, as mentioned above. I can't think of a use case where they would, but it's possible. If you decide to take this PR, perhaps make it a minor dot release?

YungTosti commented 7 years ago

Will this ever be added? Library currently does not appear to be working with an Immutable state so it won't work for us... :(

avindra commented 5 years ago

This probably should not be merged... there are consumers (like myself) who do not use immutable.js with React... and don't want it in dependencies.