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

refactor!: revert "feat: use superclass of VaadinServlet/Request (#9699)" #10812

Merged
merged 3 commits into from
Apr 27, 2021

Conversation

pleku
Copy link
Contributor

@pleku pleku commented Apr 26, 2021

This reverts commit c581668, except
intentionally leaves out the protected API introduced to StaticFileServer to get a static
resource, so it may be overridden to use something else than VaadinServletService.

…ssible (#9699)"

This reverts commit c581668, except
intentionally leaves out part in PushHandler which accepts VaadinService
and the protected API introduced to StaticFileServer to get a StaticFileServer
resource, so it may be overridden to use something else than VaadinServletService.
@pleku pleku changed the title refactor!: revert "feat: use superclass of VadinServlet/Request (#9699)" refactor!: revert "feat: use superclass of VaadinServlet/Request (#9699)" Apr 26, 2021
Fixes review comment and broken compilation.
@pleku
Copy link
Contributor Author

pleku commented Apr 27, 2021

I'll fix the commit message, the PushRequestHandler + PushHandler parts were also reverted because this change also requires that VaadinServletRequest would accept VaadinService since for some reason (at least for detecting the session), PushHandler creates a VaadinServletRequest from the received AtmosphereRequest. The code (existing before contribution) is a bit off here - only a VaadinRequest would actually be needed since the PushHandler uses only VaadinService which only needs VaadinRequest, not VaadinServletRequest. But since there is no way to just make an instance of VaadinRequest, the PushHandler code is tied to VaadinServletRequest instead.

This part is reverted for now. Making PushRequestHandler and PushHandler not rely on VaadinServletService is doable separately later on, but not a priority. It should be enough to make some kind of wrapper for VaadinRequest EDIT: actually at the moment the is no case where Atmosphere support would be used without servlet

@pleku pleku enabled auto-merge (squash) April 27, 2021 12:40
@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 6 issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. MAJOR VaadinService.java#L413: Rename "instantiator" which hides the field declared at line 192. rule
  2. MINOR VaadinService.java#L354: Replace this lambda with a method reference. rule
  3. MINOR VaadinService.java#L416: Remove this use of "init"; it is deprecated. rule
  4. MINOR VaadinService.java#L430: Remove this use of "init"; it is deprecated. rule
  5. MINOR VaadinServlet.java#L196: Remove the declaration of thrown exception 'javax.servlet.ServletException', as it cannot be thrown from method's body. rule
  6. MINOR VaadinServlet.java#L545: Replace this lambda with a method reference. rule

@pleku pleku merged commit 9f903f8 into master Apr 27, 2021
@pleku pleku deleted the fix/revert-9699 branch April 27, 2021 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants