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

Add RPC support for composite components #4298

Merged
merged 3 commits into from
Jun 28, 2018

Conversation

heruan
Copy link
Member

@heruan heruan commented Jun 17, 2018

Fixes #4198


This change is Reviewable

@Legioth
Copy link
Member

Legioth commented Jun 18, 2018

Review status: 1 unresolved discussion, 0 of 1 LGTMs obtained


flow-server/src/main/java/com/vaadin/flow/server/communication/rpc/PublishedServerEventHandlerRpcHandler.java, line 139 at r1 (raw file):

        } else if (methods.size() == 1) {
            invokeMethod(instance, methods.get(0), args);
        } else if (Composite.class.equals(clazz)) {

This assumes there's only one level of composites. It's also possible (but not very common) that a composite is created out of another composite.

I'm also not sure if we should also support @ClientCallable methods on the composite class itself, and in that case also choose what to do if both the composite and the content defines a method method with the same name.


Comments from Reviewable

@heruan
Copy link
Member Author

heruan commented Jun 18, 2018

Review status: 1 unresolved discussion, 0 of 1 LGTMs obtained


flow-server/src/main/java/com/vaadin/flow/server/communication/rpc/PublishedServerEventHandlerRpcHandler.java, line 139 at r1 (raw file):

Previously, Legioth (Leif Åstrand) wrote…

This assumes there's only one level of composites. It's also possible (but not very common) that a composite is created out of another composite.

I'm also not sure if we should also support @ClientCallable methods on the composite class itself, and in that case also choose what to do if both the composite and the content defines a method method with the same name.

My intention here is to let the handler check if there's a method on the Composite first, so if the developer is importing its own client code there's still a chance to catch the call. I didn't consider multiple level of Composites though, I'll try to figure something out.


Comments from Reviewable

@heruan
Copy link
Member Author

heruan commented Jun 18, 2018

Now this supports composites of composites, with this flow:

  1. The instance class has the method: invoke that.
  2. The instance is a Composite, try to invoke on the content.
  3. If it fails, step up in the hierarchy.

@heruan
Copy link
Member Author

heruan commented Jun 18, 2018

@Legioth What do you think about adding a specific RpcMethodNotFoundException to be thrown/caught if the method is not found? Catching the IllegalStateException feels a bit risky to me there, as it might originate from other errors.

@Legioth
Copy link
Member

Legioth commented Jun 18, 2018

Using any exception for control flow sounds wrong to me. What about splitting out a separate method that finds a method to invoke (potentially recursively)?

@heruan
Copy link
Member Author

heruan commented Jun 18, 2018

Now a new method looks for the target method in the class hierarchy; if not found and instance is a Composite it starts over with the composite content.

@heruan
Copy link
Member Author

heruan commented Jun 19, 2018

Do you feel this needs other improvements, or tests?

@Legioth Legioth added this to the 1.0.1 milestone Jun 20, 2018
@Legioth
Copy link
Member

Legioth commented Jun 20, 2018

Seems fine to me, but won't merge this right away to reduce the risk of further regressions during the release candidate period.

@denis-anisimov
Copy link
Contributor

:lgtm:


Review status: all discussions resolved, 0 of 1 LGTMs obtained, and 1 stale


Comments from Reviewable

@denis-anisimov denis-anisimov merged commit 7b0c175 into vaadin:master Jun 28, 2018
gilberto-torrezan pushed a commit that referenced this pull request Jul 11, 2018
* Add RPC support for composite components

* Support composite of composite

* Split method search in hierarchy
gilberto-torrezan pushed a commit that referenced this pull request Jul 11, 2018
* Add RPC support for composite components

* Support composite of composite

* Split method search in hierarchy
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