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

Provide a state object in the command request that can be directly modified to update store state #213

Merged
merged 14 commits into from Feb 15, 2019

Conversation

@maier49
Copy link
Contributor

@maier49 maier49 commented Jan 4, 2019

Type: feature

The following has been addressed in the PR:

Description:

This leverages proxies (where available), to add a state property to the command request object which can be modified directly to create operations that will update the store. Of the concerns raised on the original issue performance seems to be the biggest one, at least using this implementation. A simple operation that modifies one property two levels deep in an object takes about twice as long due to the time to create the proxy, regardless of whether it's used. Using the proxy slows down the operation further.

Resolves #40

@maier49 maier49 requested review from agubler and matt-gadd Jan 4, 2019
@@ -29,7 +32,7 @@ export interface CommandFactory<T = any, P extends object = DefaultPayload> {
* Command that returns patch operations based on the command request
*/
export interface Command<T = any, P extends object = DefaultPayload> {
(request: CommandRequest<T, P>): Promise<PatchOperation<T>[]> | PatchOperation<T>[];
(request: CommandRequest<T, P>): Promise<PatchOperation<T>[]> | PatchOperation<T>[] | void;
Copy link
Member

@agubler agubler Jan 9, 2019

Choose a reason for hiding this comment

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

I think this needs to be void | Promise<void> for async commands when using the state object.

Copy link
Member

@agubler agubler Feb 14, 2019

Choose a reason for hiding this comment

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

@maier49 Did you see this comment?

Copy link
Contributor Author

@maier49 maier49 Feb 15, 2019

Choose a reason for hiding this comment

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

@agubler Could have sworn I did that, sorry. It's done now!

@maier49 maier49 force-pushed the 40-proxy-adapter branch from 96413da to 960aa05 Jan 21, 2019
@maier49
Copy link
Contributor Author

@maier49 maier49 commented Jan 21, 2019

I updated this to use a getter for state and throw an exception if used in IE. It seemed preferable to an alternative factory to me for a couple reasons:

  • The user will still be informed that this is not usable in IE as soon as they run the code
  • There's no risk of accidentally using the wrong factory in other browsers and not realizing the state object is available
  • It allows the apply API to be consistent.

Because of the way the patches are applied providing a state object in IE and performing a diff on it would be extremely slow, as we'd have to recursively build and diff it, and then still create the patch operations to do all those changes all over again. It would also require changing or violating the store API as we'd need root access to the state object.

@maier49
Copy link
Contributor Author

@maier49 maier49 commented Jan 26, 2019

@matt-gadd @agubler It's not pretty, but I think I've got optional types working with both approaches now.

@agubler
Copy link
Member

@agubler agubler commented Jan 27, 2019

Nice work @maier49

@maier49 maier49 force-pushed the 40-proxy-adapter branch from 91a6b71 to 0c6f46e Feb 12, 2019
@maier49 maier49 changed the title 40 proxy adapter Provide a state object in the command request that can be directly modified to update store state Feb 15, 2019
@maier49 maier49 merged commit 8fed1da into dojo:master Feb 15, 2019
4 checks passed
@maier49 maier49 deleted the 40-proxy-adapter branch Feb 15, 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

2 participants