-
Notifications
You must be signed in to change notification settings - Fork 167
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
fix: treat "clear all" as permanent initial state #10128
Conversation
public class TestFormIT extends ChromeBrowserTest { | ||
|
||
@Test | ||
public void reattachedTemplateHasItsInitialText() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering by this test name.. please ignore if totally out of scope. just wondering. Does that mean the following:
// already attached to the client
testForm.getDiv().setText("Bar");
// already attached client side is updated to "Bar"
// remove TestForm from client
// reattach TestForm to client
Does TestForm has "Template text" or "Bar" on the client? From the test name I would expect "Template text" but from my "state model understanding" I would think it should be "Bar"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no "Bar" in the this scenario.
The original ticket is about duplicated text.
So "Template text" will be duplicated after reattach.
There is an implicit call setText("Template text")
.
May be you are right that I should avoid implicit setText
and call explicitely setText("foo"
and check that this is shown in the result...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks that makes the intention more clear and additionally checks that the state is stored in between attachments and not "overwritten" when reattached.
flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeList.java
Show resolved
Hide resolved
flow-server/src/main/java/com/vaadin/flow/internal/nodefeature/NodeList.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not certain whether we need to update the javadoc for generateChangesFromEmpty()
.
From a modified implementation point of view, it now adds a clear change even if the values are empty.
But Denis's explanations looks reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When that instance is then detached in one round trip and attached again in another round trip, then without this PR it will show the "Hello" text because there will be a new client-side instance that runs firstUpdated again but there won't be any clear event from the server. With this PR, a clear event is sent which leads to an empty element.
Apart from the knowledge on how the state nodes work under the hood, I would say that having empty text after first attach, and, then, having "Hello" text after subsequent attach/reattach, would be confusing and opens a gates for bugs, and, as Denis said, a faulty costumer-side code which relies on that behaviour. On the other hand, having the clear all state constantly looks more reasonable, because this behaviour at least expectable.
I think it should be just mentioned in the release notes.
Yes, and I believe we shouldn't maybe release this fix in the flow 2.5 immediately, but, instead, have some test period in latest flow versions to be sure that this would not cause a new regressions.
* fix: treat "clear all" as permanent initial state fixes #10119
* fix: treat "clear all" as permanent initial state fixes #10119
* fix: treat "clear all" as permanent initial state fixes #10119
* fix: treat "clear all" as permanent initial state fixes #10119
* fix: treat "clear all" as permanent initial state fixes #10119
* fix: treat "clear all" as permanent initial state fixes #10119
* fix: treat "clear all" as permanent initial state fixes #10119
* fix: treat "clear all" as permanent initial state fixes #10119
* fix: treat "clear all" as permanent initial state fixes #10119
fixes #10119