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

Don't send model properties to the server if they are not updatable #3498

Merged
merged 14 commits into from
Feb 7, 2018

Conversation

denis-anisimov
Copy link
Contributor

@denis-anisimov denis-anisimov commented Feb 6, 2018

Fix for #3425


This change is Reviewable

@denis-anisimov denis-anisimov changed the title Don't send model properties to the server if they are not updatable WIP: Don't send model properties to the server if they are not updatable Feb 7, 2018
@denis-anisimov denis-anisimov changed the title WIP: Don't send model properties to the server if they are not updatable Don't send model properties to the server if they are not updatable Feb 7, 2018
@SomeoneToIgnore
Copy link
Contributor

Reviewed 2 of 6 files at r1, 11 of 12 files at r2, 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/PolymerTemplate.java, line 209 at r3 (raw file):

        List<String> propertyNames = removeSimpleProperties();

        // This has to be executed BEFORE model population to be able to know

Is it possible to unite this and the next JS calls into one method call?
So you don't have to care about the order of the calls?


Comments from Reviewable

@denis-anisimov
Copy link
Contributor Author

Review status: 13 of 17 files reviewed at latest revision, 1 unresolved discussion.


flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/PolymerTemplate.java, line 209 at r3 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

Is it possible to unite this and the next JS calls into one method call?
So you don't have to care about the order of the calls?

Just let me know how.


Comments from Reviewable

@SomeoneToIgnore
Copy link
Contributor

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/PolymerTemplate.java, line 209 at r3 (raw file):

Previously, denis-anisimov (Denis) wrote…

Just let me know how.

To me it is.


Comments from Reviewable

@denis-anisimov
Copy link
Contributor Author

Review status: 13 of 17 files reviewed at latest revision, all discussions resolved.


flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/PolymerTemplate.java, line 209 at r3 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

To me it is.

Just let me know how.


Comments from Reviewable

@SomeoneToIgnore
Copy link
Contributor

flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/PolymerTemplate.java, line 209 at r3 (raw file):

Previously, denis-anisimov (Denis) wrote…

Just let me know how.

Well, can you unite two methods into one, get three parameters in it and do all the required stuff in the order required there, can't you?


Comments from Reviewable

@denis-anisimov
Copy link
Contributor Author

Review status: 13 of 17 files reviewed at latest revision, all discussions resolved.


flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/PolymerTemplate.java, line 209 at r3 (raw file):

Previously, SomeoneToIgnore (Kirill Bulatov) wrote…

Well, can you unite two methods into one, get three parameters in it and do all the required stuff in the order required there, can't you?

Ah, sorry. Wrong place of the code. Thought that this is client side code..........
(I'm so tired from this crappy solutions).

Well......
Has sense.
The result will be most likely two more days of fixing tests: now: there are two independent public methods on the client side which are tested via the GWT tests.
In the result there will be one method which will needs tests adoption.

Let's say : I don't want to fix more GWT tests in this patch anymore.
I spent more than 3 days on this shit.
I will create a ticket for this improvement.


Comments from Reviewable

@denis-anisimov
Copy link
Contributor Author

Review status: 13 of 17 files reviewed at latest revision, all discussions resolved, some commit checks failed.


flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/PolymerTemplate.java, line 209 at r3 (raw file):

Previously, denis-anisimov (Denis) wrote…

Ah, sorry. Wrong place of the code. Thought that this is client side code..........
(I'm so tired from this crappy solutions).

Well......
Has sense.
The result will be most likely two more days of fixing tests: now: there are two independent public methods on the client side which are tested via the GWT tests.
In the result there will be one method which will needs tests adoption.

Let's say : I don't want to fix more GWT tests in this patch anymore.
I spent more than 3 days on this shit.
I will create a ticket for this improvement.

#3502


Comments from Reviewable

@SomeoneToIgnore
Copy link
Contributor

Review status: 13 of 17 files reviewed at latest revision, 1 unresolved discussion.


flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/PolymerTemplate.java, line 209 at r3 (raw file):

Previously, denis-anisimov (Denis) wrote…

#3502

Oh, I see, disregard the comment then :)
Thanks.


Comments from Reviewable

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 5 issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. MAJOR ExecuteJavaScriptProcessor.java#L177: Remove this unused private "handleRemoveExistingNode" method. rule
  2. MAJOR ExecuteJavaScriptProcessor.java#L186: Remove this unused private "getAppId" method. rule
  3. MAJOR StateNode.java#L247: Suspicious comparison of Boolean references in com.vaadin.client.flow.StateNode.lambda$setDomNode$3(Function) rule
  4. MAJOR SimpleElementBindingStrategy.java#: This file has 842 lines, which is greater than 750 authorized. Split it into smaller files. rule
  5. MAJOR SimpleElementBindingStrategy.java#L777: Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed. rule

@SomeoneToIgnore
Copy link
Contributor

Reviewed 6 of 6 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@SomeoneToIgnore SomeoneToIgnore merged commit 9e93a43 into master Feb 7, 2018
@denis-anisimov denis-anisimov deleted the 3425-polymer-properties-sync branch February 7, 2018 11:11
@gilberto-torrezan gilberto-torrezan added this to the 1.0.0.alpha20 milestone Feb 8, 2018
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