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

Subproperties in template models do not get updated on the server #3117

Closed
marcushellberg opened this issue Dec 7, 2017 · 12 comments
Closed
Assignees
Labels
Milestone

Comments

@marcushellberg
Copy link
Member

Flow version: 1.0.0.alpha11

When using an object in a TemplateModel and binding to sub properties in the template, I expect to be able to access the updated values in an eventhandler method.

Relevant parts of the code:

Java:

public interface Model extends TemplateModel {
  void setOrder(TShirtOrder order);
  TShirtOrder getOrder();
}

@EventHandler
public void submit() {
  service.placeOrder(getModel.getOrder();
}

Template:

<vaadin-text-field label="Name" value="{{order.name}}>
<vaadin-text-field label="Email" value="{{order.email}}>

I can verify that the submit method does get called and that the order property in the Polymer component has been updated correctly.

@pleku pleku added the bug label Dec 23, 2017
@caalador
Copy link
Contributor

caalador commented Jan 5, 2018

Could you re-test or give a more complete example.
Worked on my simple test setup.

@marcushellberg
Copy link
Member Author

I updated to latest flow/vaadin and i still seem to have the same issue. The project is our tshirt demo that we use at conferences: https://github.com/vaadin/tshirt-shop-flow

@caalador
Copy link
Contributor

caalador commented Jan 8, 2018

I can't see that repository. I only see -vaadin and -polymer

@marcushellberg
Copy link
Member Author

Try now.

@caalador
Copy link
Contributor

caalador commented Jan 8, 2018

Now I see it.
I'll test with it and try to figure out what is different from my simple test.
I have an itch that it might be a timing issue, but I'll look into it.

@caalador caalador self-assigned this Jan 8, 2018
@caalador
Copy link
Contributor

caalador commented Jan 8, 2018

This took me way too long to figure out. The problem is that the getModel().getOrder() returns a proxy object which means that the values are not reflected to the fields in the object, but will be returned if you use the getters. So the problem is fixed by changing the submit event to be:

    @EventHandler
    public void submit() {
        TShirtOrder order = getModel().getOrder();
        service.placeOrder(new TShirtOrder(order.getName(), order.getEmail(), order.getShirtSize()));
    }

@pleku
Copy link
Contributor

pleku commented Jan 8, 2018

We should probably at least highlight his use case as an example in the tutorial. It is a gotcha that can cause lots of frustration and lost time for users.

@caalador
Copy link
Contributor

caalador commented Jan 8, 2018

From the template-model-bean tutorial:

[NOTE]
Your bean will never be stored as a bean in the model, instead the individual parts of the bean will be stored. No method will ever return the original bean to you.

[NOTE]
The proxy bean returned by the getter is not meant to be passed on to an `EntityManager` or similar. It is purely meant for updating the values in the model.

@marcushellberg
Copy link
Member Author

Thanks, that explains it.

I read those sentences a few times while trying to figure this out, but I understood it that I just won't get the same instance of the bean back. It was unexpected that I got an object that looks like the one I want, but doesn't contain the data I updated.

I think that this will be something many will run into, so let's see if we can either have it somehow return a populated object, or at least document it more clearly.

@Legioth
Copy link
Member

Legioth commented Jan 8, 2018

The reason for not returning the original object is to be able to know that the developer has been running setters on the instance so that Flow can send those updated values to the client. This is an alternative solution to the problem that Polymer has "solved" by forcing you to do set("foo.bar", "value") when setting sub properties.

Each approach requires the developer to be aware of the situation. I'm not aware of any good way of getting rid of both of the drawbacks at once (the bad way that I'm aware of is the FW7 way of keeping a separate copy of the data and scheduling a dirty check any time you've done getState()).

@marcushellberg
Copy link
Member Author

Sure. I think that's fine as long as we make it clear to the developer. Perhaps I read the docs too quickly, but I still did wrong even after reading them. Would be good if we could try to make it even more apparent for developers to avoid frustration down the line.

@caalador
Copy link
Contributor

caalador commented Jan 9, 2018

Yes. The documentation should probably be changed somehow, so if you have an idea how it should be reworded or a sample to highlight this, it would be welcome.

@pleku pleku closed this as completed Jan 12, 2018
@pleku pleku added this to the 1.0.0.alpha18 milestone Jan 12, 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

4 participants