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!: Remove StaticFileHandler.isStaticResourceRequest #12263

Merged
merged 12 commits into from Nov 8, 2021

Conversation

Artur-
Copy link
Member

@Artur- Artur- commented Nov 3, 2021

This simplifies static file handling so the same logic is not duplicate in two places. It also makes it possible to always query the dev server for files instead of knowing upfront what the dev server can serve.

The method is removed instead of retained as there are two ways it could have been used:

  1. Called to check if a given request is a static resource request
  2. Overridden in a custom StaticFileHandler

To keep the method we would need to do the following for the cases:

  1. Make isStaticResourceRequest return serveStaticResource() != null
  2. Make serveStaticResource() call isStaticResourceRequest() as the first thing

We cannot do both so it is safer to remove the method to force code to be updated

@pleku
Copy link
Contributor

pleku commented Nov 4, 2021

Even if it is unlikely that anyone is using this API, should it be marked as refactor!: so that it gets mentioned in the release notes ?

@Artur- Artur- changed the title refactor: Delete StaticFileHandler.isStaticResourceRequest refactor!: Delete StaticFileHandler.isStaticResourceRequest Nov 4, 2021
@knoobie
Copy link
Contributor

knoobie commented Nov 4, 2021

Isn't the vaadin website using this API? Looking at the PR / feature request that introduced this feature? #4914

It's possible that some users have used this API for the exact same thing.

@Artur- Artur- changed the title refactor!: Delete StaticFileHandler.isStaticResourceRequest refactor!: Remove StaticFileHandler.isStaticResourceRequest Nov 4, 2021
@Artur-
Copy link
Member Author

Artur- commented Nov 4, 2021

It is still possible to override it in the same way. Only it should be simpler because there is one method and not two

This simplifies static file handling so the same logic is not duplicate in two places. It also makes it possible to always query the dev server for files instead of knowing upfront what the dev server can serve.

The method is removed instead of retained as there are two ways it could have been used:
1. Called to check if a given request is a static resource request
2. Overridden in a custom StaticFileHandler

To keep the method we would need to do the following for the cases:
1. Make isStaticResourceRequest return serveStaticResource() != null
2. Make serveStaticResource() call isStaticResourceRequest() as the first thing

We cannot do both so it is safer to remove the method to force code to be updated
@Artur- Artur- enabled auto-merge (squash) November 5, 2021 09:22
@Artur- Artur- added the vite Tickets related to vite support label Nov 5, 2021
@Artur- Artur- added this to In progress in Frontend build optimization via automation Nov 5, 2021
@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 14 issues

  • MAJOR 7 major
  • MINOR 7 minor

Top 10 issues

  1. MAJOR StaticFileServer.java#L127: Replace this with a call to the "toFile().isDirectory()" method rule
  2. MAJOR StaticFileServer.java#L137: Replace this with a call to the "toFile().isDirectory()" method rule
  3. MAJOR AbstractDevServerRunner.java#L114: Make "devServerStartFuture" transient or serializable. rule
  4. MAJOR AbstractDevServerRunner.java#L191: Invoke method(s) only conditionally. rule
  5. MAJOR AbstractDevServerRunner.java#L382: Either re-interrupt this method or rethrow the "InterruptedException". rule
  6. MAJOR AbstractDevServerRunner.java#L612: Null passed for non-null parameter of java.util.concurrent.CompletableFuture.getNow(Object) in com.vaadin.base.devserver.AbstractDevServerRunner.handleRequest(VaadinSession, VaadinRequest, VaadinResponse) rule
  7. MAJOR AbstractDevServerRunner.java#L673: Refactor this method to reduce its Cognitive Complexity from 20 to the 15 allowed. rule
  8. MINOR StaticFileServer.java#L178: Move the "file" string literal on the left side of this string comparison. rule
  9. MINOR VaadinServlet.java#L196: Remove the declaration of thrown exception 'javax.servlet.ServletException', as it cannot be thrown from method's body. rule
  10. MINOR VaadinServlet.java#L331: Replace this if-then-else statement by a single return statement. rule

@Artur- Artur- merged commit fdbe642 into master Nov 8, 2021
@Artur- Artur- deleted the delete-isstaticresource branch November 8, 2021 07:44
Frontend build optimization automation moved this from In progress to Done Nov 8, 2021
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 22.0.0.beta1 and is also targeting the upcoming stable 22.0.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants