Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Immutable support #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Immutable support #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea86d3f
use immutable for reducer state
zeevl Nov 1, 2016
4999a3e
updated tests
zeevl Nov 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion example/src/components/container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ Container.contextTypes = {
};

Container.propTypes = {
notifications: PropTypes.array
notifications: PropTypes.object
};

export default connect(
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"redux": "^3.5.2"
},
"dependencies": {
"immutable": "^3.8.1",
"react": "^0.14 || ^15.0.0-rc || ^15.0",
"react-dom": "^0.14 || ^15.0.0-rc || ^15.0",
"react-notification-system": "^0.2.7"
Expand Down
4 changes: 2 additions & 2 deletions src/__tests__/notifications.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ describe('NotificationsComponent', () => {
expect(wrapper.children()).toBeDefined();
});

it('should warn if prop:notifications is not array', () => {
it('should warn if prop:notifications is not object', () => {
spyOn(console, 'error');

const wrapper = shallow(<Component notifications={1} />);
const warning = console.error.calls.argsFor(0)[0];

expect(warning).toMatch(/Invalid prop `notifications` of type `number` supplied to `Notifications`, expected `array`./);
expect(warning).toMatch(/Invalid prop `notifications` of type `number` supplied to `Notifications`, expected `object`./);
});
});
13 changes: 7 additions & 6 deletions src/__tests__/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,26 @@
import { List, toJS } from 'immutable'
import Reducer from '../reducer';
import * as Actions from '../actions';

describe('reducer', () => {
it('initializes state with an array', () => {
expect(Reducer()).toEqual([]);
expect(Reducer().toJS()).toEqual([]);
});

it('stores the notification to state', () => {
const action = Actions.success();
const state = Reducer([], action);
const state = Reducer(List([]), action);

expect(state.length).toEqual(1);
expect(state.size).toEqual(1);
});

it('stores and removes notification', () => {
const uid = 1;

const state = Reducer([], Actions.success({ uid }));
expect(state.length).toEqual(1);
const state = Reducer(List([]), Actions.success({ uid }));
expect(state.size).toEqual(1);

const newState = Reducer(state, Actions.hide(uid));
expect(newState.length).toEqual(0);
expect(newState.size).toEqual(0);
});
});
4 changes: 2 additions & 2 deletions src/notifications.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class Notifications extends React.Component {
}

componentWillReceiveProps(nextProps) {
const {notifications} = nextProps;
const notifications = nextProps.notifications.toJS();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ☕️

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@zeevl zeevl Nov 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heya @zeevl @AverageZ

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@alameya alameya Dec 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const notificationIds = notifications.map(notification => notification.uid);

// Get all active notifications from react-notification-system
Expand Down Expand Up @@ -48,7 +48,7 @@ class Notifications extends React.Component {
}

Notifications.propTypes = {
notifications: PropTypes.array
notifications: PropTypes.object
};

Notifications.contextTypes = {
Expand Down
9 changes: 4 additions & 5 deletions src/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
import { List } from 'immutable';
import {RNS_SHOW_NOTIFICATION, RNS_HIDE_NOTIFICATION} from './const';

export default function Notifications(state = [], action = {}) {
export default function Notifications(state = List([]), action = {}) {
switch(action.type) {
case RNS_SHOW_NOTIFICATION:
const { type, ...rest } = action;
return [
...state,
{ ...rest, uid: action.uid}
];
return state.push({ ...rest, uid: action.uid });

case RNS_HIDE_NOTIFICATION:
return state.filter(notification => {
return notification.uid !== action.uid;
Expand Down