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

Disallow Component to implement AppShellConfigurator #18012

Closed
mvysny opened this issue Nov 9, 2023 · 2 comments · Fixed by #18169
Closed

Disallow Component to implement AppShellConfigurator #18012

mvysny opened this issue Nov 9, 2023 · 2 comments · Fixed by #18169

Comments

@mvysny
Copy link
Member

mvysny commented Nov 9, 2023

Describe your motivation

Certain customers love to take these kinds of shortcuts:

@Route("")
@Theme("my-theme")
public class MainView extends VerticalLayout implements AppShellConfigurator {

    public MainView() {
        Notification.show("foo"); // fails
    }
}

The problem with the code above is that the customer successfully shoots himself in the foot, the code fails with an exception, and it's not clear why:

Caused by: java.lang.IllegalArgumentException: Unable to create an instance of 'com.vaadin.starter.skeleton.MainView'. The constructor threw an exception.
	at com.vaadin.flow.internal.ReflectTools.createProxyInstance(ReflectTools.java:515)
	at com.vaadin.flow.internal.ReflectTools.createInstance(ReflectTools.java:452)
	at com.vaadin.flow.di.DefaultInstantiator.create(DefaultInstantiator.java:134)
	at com.vaadin.flow.di.DefaultInstantiator.getOrCreate(DefaultInstantiator.java:63)
	at com.vaadin.flow.server.AppShellRegistry.modifyIndexHtml(AppShellRegistry.java:228)
	at com.vaadin.flow.server.communication.IndexHtmlRequestHandler.synchronizedHandleRequest(IndexHtmlRequestHandler.java:149)
	at com.vaadin.flow.server.SynchronizedRequestHandler.handleRequest(SynchronizedRequestHandler.java:40)
	at com.vaadin.flow.server.VaadinService.handleRequest(VaadinService.java:1522)
	... 24 more
Caused by: java.lang.reflect.InvocationTargetException
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480)
	at com.vaadin.flow.internal.ReflectTools.createProxyInstance(ReflectTools.java:484)
	... 31 more
Caused by: java.lang.IllegalStateException: UI instance is not available. It means that you are calling this method out of a normal workflow where it's always implicitly set. That may happen if you call the method from the custom thread without 'UI::access' or from tests without proper initialization.
	at com.vaadin.flow.component.notification.Notification.setOpened(Notification.java:404)
	at com.vaadin.flow.component.notification.Notification.open(Notification.java:330)
	at com.vaadin.flow.component.notification.Notification.show(Notification.java:252)
	at com.vaadin.flow.component.notification.Notification.show(Notification.java:269)
	at com.vaadin.starter.skeleton.MainView.<init>(MainView.java:17)
	... 37 more

The technical problem above is that AppShellConfigurator is instantiated early, when the UI hasn't been constructed yet. That means that the UI is null and the notification can not be shown.

The main problem is a design one: the developer is trying to put too many responsibilities on one class, which I don't think is a good pattern. In such situation main layout is instantiated two times:

  1. First as AppShell, with UI being null. It's only used to configure app shell, then is thrown away. The fact that it's a Vaadin component is completely ignored.
  2. Second as a regular route, upon navigation. In this scenario, the AppShell functionality is completely ignored.

We should disallow a programmer from taking such shortcuts.

Describe the solution you'd like

A correct behavior would actually be throwing an exception on startup if you use AppShellConfigurator on a class that extends a Component, or has any @Route annotations or such.

Describe alternatives you've considered

Alternatively we could accept this and just document this behavior.

@mvysny
Copy link
Member Author

mvysny commented Nov 30, 2023

Acceptance criteria: a Vaadin app with the following class should fail to start; an InvalidApplicationConfigurationException should be thrown.

public class AppShell extends VerticalLayout implements AppShellConfigurator{}

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.4.0.alpha1 and is also targeting the upcoming stable 24.4.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Vaadin Flow enhancements backlog (Vaa...
  
Done / Pending Release
Development

Successfully merging a pull request may close this issue.

3 participants