Factor out a MutableState interface from the store and provide an ImmutableJS implementation
#242
Conversation
|
Looks like this version of immutable has different behavior in IE than other browsers so I'll have to sort that out |
|
Switched to using the latest fully fledged version of Immutable which doesn't suffer from the same issue. |
| "css-select-umd": "1.3.0-rc0", | ||
| "diff": "3.5.0", | ||
| "globalize": "1.4.0", | ||
| "immutable": "3.8.2", |
There was a problem hiding this comment.
Should we include immutable as a dependency, or make it a dev dependency and also an optional dependency?
There was a problem hiding this comment.
I made it an optional and dev dependency. In the code I just wrapped the require in a try. I'm assuming that we expect that module will fail if Immutable isn't installed, but I'm not sure how we want to handle that. I didn't see anywhere else we're using optional dependencies so let me know if that's the wrong approach.
There was a problem hiding this comment.
Perhaps we should put something in the README to say that they would need to install immutable.... Or maybe having it as a dev isn't the end of the world... it's not going to affect anything unless they use it anyway.
There was a problem hiding this comment.
Your call. Since it should be left out if they're not using it I'd be fine with it being a dependency.
There was a problem hiding this comment.
yeah let's put it in the deps again. sorry!
There was a problem hiding this comment.
No problem. Reverted
maier49
force-pushed
the
48-patch-performance
branch
from
daf0e8d to
b0c7b23
Compare
maier49
changed the title
MutableState interface from the store and provide an ImmutableJS implementation
src/stores/README.md
Outdated
|
|
||
| #### ImmutableState | ||
|
|
||
| An implementation of the `MutableState` interface that leverages [Immutable](https://github.com/immutable-js/immutable-js) under the hood is provided as |
There was a problem hiding this comment.
Is this formatting a little off with line endings?
There was a problem hiding this comment.
I moved it to one line to avoid any issues.
| global.localStorage.setItem(LOCAL_STORAGE_TEST_ID, '[{"meta":{"path":"/counter"},"state":1}]'); | ||
| load(LOCAL_STORAGE_TEST_ID, store); | ||
| assert.deepEqual((store as any)._state, { counter: 1 }); | ||
| assert.deepEqual((store as any)._state._state, { counter: 1 }); |
There was a problem hiding this comment.
A side effect of the impl is having state stored in a nested _state._state object. Do you think that it should probably be something like _adapter._state?
There was a problem hiding this comment.
_adapter seems reasonable to me. I've updated it
3 participants
Type: feature
The following has been addressed in the PR:
prettieras per the readme code style guidelinesDescription:
Related to #48
This creates a
MutableStateinterface that has the properties ofStateas well as theapplyfunction. It then breaks out the parts of store that implemented theMutableStateinterface into a default implementation, and allows an alternative implementation to be passed in to the constructor.Includes an implementation using ImmutableJS. So far testing the performance impact of using ImmutableJS has not shown significant differences but I have not thoroughly tested it.