Immutable support

[zeevl]

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]

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]

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?

[gor181]

hey @zeevl ,

What happens if I'm not using immutable-js? This would fail right?
As native arrays do not have toJs method?

I have a feeling that if we merge this in then it will break all the existing apps using the lib?
Could be that I need more ☕️

[AverageZ]

Might it work if you used map()?

Both native arrays and Immutable List have the map method (plus decent support for native array.map, IE9+). And you're not directly passing the List or array into the react-notification-system component so it shouldn't affect it.

Bonus would also be not having to perform the toJS() call on the notifications

[zeevl]

the change to componentWillReceiveProps is agnostic to whether the rest of the app is using immutable elsewhere or not, since we're directly connecting state.notifications to the component, i.e. (https://github.com/gor181/react-notification-system-redux/blob/master/example/src/components/container.js#L62). If a consumer follows the docs and examples, nextProps.notifications will be always be the immutable List.

Edit: take note that the example code didn't change with this PR, but still works as expected..

[zeevl]

also, @AverageZ, I don't think map() would work -- with this immutable change, the objects in the notifications list are also immutable, so they would either require a call to toObject(), toJS() some other immutable-specific api.

[gor181]

heya @zeevl @AverageZ

thanks for answers, will merge this in by beginning next week 🍻

[gor181]

Hey ppl,

Was thinking about this, and I'm not sure whether we should force people to have immutable-js pulled down as a part of this lib.

What do you think?
Maybe we can make some sort of version which includes immutable as you suggested previously?

Thanks and apologies for late reply.

[alameya]

Guys, there no need any changes for use this lib with immutableJS
Diffrence is only in external usage.

Bellow example ...

File with reducers

import {reducer as notificationReducer} from 'react-notification-system-redux';

export function reducer(state = Map({}), action) {
    return Map({
        notifications: notificationReducer(state.get('notifications', []), action),
        // app reducers
        ........
    });
}

Usage in container layer

function mapStateToProps(state, props) {
    return {
        notifications: state.get('notifications', [])
    };
}

That is it!
Usage in presentation layer has no any difference

[geoolekom]

You are definitely wrong, @alameya
"Map" creates new object each time, so you force component to rerender each time you call reducer. This is drastically decrease perfomance.

[YungTosti]

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]

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.