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

Synchronize client property changes correctly to the server #3425

Closed
SomeoneToIgnore opened this issue Jan 29, 2018 · 4 comments
Closed

Synchronize client property changes correctly to the server #3425

SomeoneToIgnore opened this issue Jan 29, 2018 · 4 comments
Assignees
Labels
Milestone

Comments

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Jan 29, 2018

Currently Flow has two ways of synchronizing properties to the server.

One synchronization happens for each polymer element:
https://github.com/vaadin/flow/blob/master/flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java#L264

every property that is defined on the server side is synchronized on each change on the client.

The other synchronization happens for each property that was explicitly set to be sync on some particular event (disregarding of the nature of the element: it can be polymer element, can be simple element):
https://github.com/vaadin/flow/blob/master/flow-client/src/main/java/com/vaadin/client/flow/binding/SimpleElementBindingStrategy.java#L602

The problem with the approach is that now, for Polymer elements, there is no way to make any property being synchronized on some particular event: the first synchronization will update it always, on any change.

To avoid this, a stub was introduced in #3420
Now first synchronization is skipped, if the property changed is set to be synced on some particular event.

Although working currently, this is not the correct way to do it: instead, we don't need to synchronize every property for polymer object in the first case at all: we need to synchronize the model properties only in the first case.

But implementing it this way will break some existing components in Flow tests at least, for instance: https://github.com/vaadin/flow/blob/1a495adefbbc4afcfb9d0b30a67c2f7ce8e5fc61/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/webcomponent/PaperSliderIT.java

The solution to such cases is to synchronize properties required manually.

Also, the stub introduced in #3420 should be removed.

@SomeoneToIgnore
Copy link
Contributor Author

As @Legioth suggested, it can be done by sending an extra info to the client about the template presence: since the model is present for templates only, we can use this information and sync automatically only properties that are relevant to Polymer elements with templates.
This still does not fix the problem for Polymer elements with templates, the rest will be handled by #1331

As an alternative suggestion from @denis-anisimov , we should write all the model properties info similar to whe way we do it partially in

into separate list and check this list instead.

@denis-anisimov
Copy link
Contributor

denis-anisimov commented Jan 29, 2018

The solution to such cases is to synchronize properties required manually.

I don't think it's the solution.
The updates for model properties should always be sent to the server regardless of the synchronization.

So the code should work in the same way as it worked before the change in #3420 but the code which listen all property changes should work for only model properties .
Now it works for any property in polymer element.

There is at least one reason to do that: we can't synchronize subproperties. And we want to get updates for any subproperty change.

So no, let's not synchronize explicitly every model property.
Instead the code which listens the polymer properties updated should be called only for the property if it's a model property.

@SomeoneToIgnore
Copy link
Contributor Author

Right, that's what I've meant, thanks for clarification.
So

  • synchronize model properties always
  • synchronize all other properties on demand only

@denis-anisimov
Copy link
Contributor

synchronize has too many meanings.

You mean by synchronize that the change should be sent to the server.
Let's keep synchronize term only for the properties that are really @Synchronized .
Otherwise we are confusing each other.

@denis-anisimov denis-anisimov self-assigned this Feb 2, 2018
denis-anisimov pushed a commit that referenced this issue Feb 2, 2018
denis-anisimov pushed a commit that referenced this issue Feb 2, 2018
* Add a generic way to store data inside StateNode

Part of the fix for #3425

* Gwt unit test and rename method
denis-anisimov pushed a commit that referenced this issue Feb 5, 2018
@pleku pleku closed this as completed Feb 9, 2018
@pleku pleku added this to the 1.0.0.alpha20 milestone Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants