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

Implement resynchronization of the client and server #7668

Merged
merged 5 commits into from
Feb 26, 2020

Conversation

joheriks
Copy link
Contributor

@joheriks joheriks commented Feb 25, 2020

Adapted by removing the rejection of stale promises (2.1 does not support @ClientCallable return values).


This change is Reviewable

* Mark server-side state tree unattached and send all changes, clear client-side state tree and rebuild DOM 

* Reject any pending promises from @ClientCallable invocations during client resynchronization
Copy link
Contributor

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 12 files at r1, 4 of 4 files at r3.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)


flow-server/src/main/java/com/vaadin/flow/internal/StateTree.java, line 437 at r3 (raw file):

public void prepareForResync() 

Is this impl the same as in the original PR ?

I would move this logic into the Root node as a whole method and avoid exposing fireDetachListeners and fireAttachListeners

…achListeners and StateNode::fireDetachListeners
Copy link
Contributor Author

@joheriks joheriks left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)


flow-server/src/main/java/com/vaadin/flow/internal/StateTree.java, line 437 at r3 (raw file):

Previously, denis-anisimov (Denis) wrote…
public void prepareForResync() 

Is this impl the same as in the original PR ?

I would move this logic into the Root node as a whole method and avoid exposing fireDetachListeners and fireAttachListeners

Done.

denis-anisimov
denis-anisimov previously approved these changes Feb 25, 2020
Copy link
Contributor

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r4.
Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained


flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 395 at r4 (raw file):

public void prepareForResync() {

Sorry , I meant RootNode but it's inside StateTree and it doesn't help....
I would may be do this method protected,
keep the same method in the StateTree which delegates to RootNode::prepareForResync.
But may be it's not worth .......
Up to you .

Copy link
Contributor Author

@joheriks joheriks left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained


flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java, line 395 at r4 (raw file):

Previously, denis-anisimov (Denis) wrote…
public void prepareForResync() {

Sorry , I meant RootNode but it's inside StateTree and it doesn't help....
I would may be do this method protected,
keep the same method in the StateTree which delegates to RootNode::prepareForResync.
But may be it's not worth .......
Up to you .

Yes, that is better. Fixed.

Copy link
Contributor

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 5 files at r5.
Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained

@joheriks joheriks merged commit 4d08f77 into 2.1 Feb 26, 2020
@joheriks joheriks deleted the issues/fix-7590-pick-2.1 branch February 26, 2020 10:58
@jm-ferreira jm-ferreira added this to the 2.1.6 milestone Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants