Skip to content

Use immutable data structures for applab screen reducer#8585

Closed
pcardune wants to merge 1 commit into
stagingfrom
pcardune-applab-immutable-reducers
Closed

Use immutable data structures for applab screen reducer#8585
pcardune wants to merge 1 commit into
stagingfrom
pcardune-applab-immutable-reducers

Conversation

@pcardune

Copy link
Copy Markdown
Contributor


if (state.screens.currentScreenId !== lastState.screens.currentScreenId) {
renderScreens(state.screens.currentScreenId);
if (!lastState ||

@bcjordan bcjordan May 24, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One option that might help a tiny bit with scan-ability of these lines:

const screenChanged = [boolean logic];
if (screenChanged) {

(no strong feeling on this, though)

@bcjordan

Copy link
Copy Markdown
Contributor

LGTM!

@Bjvanminnen

Copy link
Copy Markdown
Contributor

Generally lgtm. Any thoughts on whether we could/should have a way of clearly indicating which reducers expose Immutable objects vs. JS objects?

@islemaster

Copy link
Copy Markdown
Contributor

image

@pcardune

Copy link
Copy Markdown
Contributor Author

@Bjvanminnen perhaps a naming convention? I would say prefix everything with immutable but that is too long. Perhaps postfix with the specific immutable type? So this would become state.screensMap.get(...)

@Bjvanminnen

Copy link
Copy Markdown
Contributor

Another option (that I don't think I like) would be to put immutable reducers in their own subtree, i.e.

state = {
  instructions: { ... },
  immutable: {
    screens: Immutable.Map({...}),
    somethingElse: Immutable.Map({...})
 }
};

It's also possible that we don't need to do anything here, and it will be reasonably clear just by looking at the reducer or other consumers of the reducer whether or not it's using Immutable.

@pcardune pcardune force-pushed the pcardune-applab-immutable-reducers branch from e2effed to ea862d0 Compare June 6, 2016 22:40
@pcardune

Copy link
Copy Markdown
Contributor Author

going to just leave this in my applab import branch

@pcardune pcardune closed this Jun 14, 2016
@pcardune pcardune deleted the pcardune-applab-immutable-reducers branch June 14, 2016 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants