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.

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

Factor out a MutableState interface from the store and provide an ImmutableJS implementation #242

Merged
merged 13 commits into from Feb 26, 2019

Conversation

@maier49
Copy link
Contributor

@maier49 maier49 commented Feb 5, 2019

Type: feature

The following has been addressed in the PR:

Description:

Related to #48

This creates a MutableState interface that has the properties of State as well as the apply function. It then breaks out the parts of store that implemented the MutableState interface 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.

@maier49 maier49 requested a review from matt-gadd Feb 6, 2019
@maier49
Copy link
Contributor Author

@maier49 maier49 commented Feb 6, 2019

Looks like this version of immutable has different behavior in IE than other browsers so I'll have to sort that out

@maier49
Copy link
Contributor Author

@maier49 maier49 commented Feb 12, 2019

Switched to using the latest fully fledged version of Immutable which doesn't suffer from the same issue.

@@ -52,6 +52,7 @@
"css-select-umd": "1.3.0-rc0",
"diff": "3.5.0",
"globalize": "1.4.0",
"immutable": "3.8.2",
Copy link
Member

@agubler agubler Feb 15, 2019

Choose a reason for hiding this comment

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

Should we include immutable as a dependency, or make it a dev dependency and also an optional dependency?

Copy link
Contributor Author

@maier49 maier49 Feb 26, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

@agubler agubler Feb 26, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@maier49 maier49 Feb 26, 2019

Choose a reason for hiding this comment

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

Your call. Since it should be left out if they're not using it I'd be fine with it being a dependency.

Copy link
Member

@agubler agubler Feb 26, 2019

Choose a reason for hiding this comment

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

yeah let's put it in the deps again. sorry!

Copy link
Contributor Author

@maier49 maier49 Feb 26, 2019

Choose a reason for hiding this comment

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

No problem. Reverted

@maier49 maier49 force-pushed the 48-patch-performance branch from daf0e8d to b0c7b23 Feb 26, 2019
@maier49 maier49 changed the title 48 patch performance Factor out a MutableState interface from the store and provide an ImmutableJS implementation Feb 26, 2019

#### ImmutableState

An implementation of the `MutableState` interface that leverages [Immutable](https://github.com/immutable-js/immutable-js) under the hood is provided as
Copy link
Member

@agubler agubler Feb 26, 2019

Choose a reason for hiding this comment

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

Is this formatting a little off with line endings?

Copy link
Contributor Author

@maier49 maier49 Feb 26, 2019

Choose a reason for hiding this comment

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

I moved it to one line to avoid any issues.

@@ -62,12 +62,12 @@ describe('middleware - local storage', (suite) => {
it('should load from local storage', () => {
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 });
Copy link
Member

@agubler agubler Feb 26, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@maier49 maier49 Feb 26, 2019

Choose a reason for hiding this comment

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

_adapter seems reasonable to me. I've updated it

@maier49 maier49 merged commit 806114b into dojo:master Feb 26, 2019
2 of 4 checks passed
@maier49 maier49 deleted the 48-patch-performance branch Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants