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

feat: Lit templates in Vite dev and prod mode #12204

Merged
merged 31 commits into from Nov 8, 2021
Merged

feat: Lit templates in Vite dev and prod mode #12204

merged 31 commits into from Nov 8, 2021

Conversation

joheriks
Copy link
Contributor

@joheriks joheriks commented Nov 1, 2021

Dev mode: load template from file system (looking into frontend, @node_modules/@vaadin/flow-frontend, @node_modules (for paths not starting with ./)).
Prod mode: copy templates to target/classes/META-INF/VAADIN/config/templates as part of build-frontend task. I.e., template sources are no longer gotten from stats.json.

@joheriks joheriks added the vite Tickets related to vite support label Nov 1, 2021
@joheriks joheriks changed the title Lit templates in dev and prod mode Lit templates in Vite dev and prod mode Nov 1, 2021
try {
FileUtils.copyFileToDirectory(source, target);
} catch (IOException e) {
e.printStackTrace();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CatchAndPrintStackTrace: Logging or rethrowing exceptions should usually be preferred to catching and calling printStackTrace
(at-me in a reply with help or ignore)

@joheriks joheriks force-pushed the vite-templates branch 2 times, most recently from b595775 to 13648a4 Compare November 1, 2021 09:05
@joheriks joheriks changed the title Lit templates in Vite dev and prod mode feat: Lit templates in Vite dev and prod mode Nov 1, 2021
@joheriks joheriks marked this pull request as ready for review November 1, 2021 12:55
Copy link
Member

@Artur- Artur- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we should not change webpack to work the same way at the same time?

@caalador caalador added this to In progress in Frontend build optimization via automation Nov 2, 2021
@joheriks joheriks force-pushed the vite-templates branch 2 times, most recently from 571caf9 to 4724805 Compare November 3, 2021 14:29
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine as long as the webpack functionality is validated on Bakery or other template heavy application.

@@ -236,7 +236,7 @@ module.exports = {
devServer: {
hot: false, // disable HMR
client: false, // disable wds client as we handle reloads and errors better
// webpack-dev-server serves ./ , webpack-generated, and java webapp
// webpack-dev-server serves ./Frontend , webpack-generated, and java webapp
static: [outputFolder, path.resolve(__dirname, 'src', 'main', 'webapp')],
onAfterSetupMiddleware: function (devServer) {
devServer.app.get(`/stats.json`, function (req, res) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stats.json and stats.hash endpoins could be removed as they are not used anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also stats plugin can be removed and only have the generated stats.json contain the result from ?stats.assetsByChunkName but this cleanup can be in a different ticket that should be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Ticket for the remaining refactoring here: #12301

import com.vaadin.flow.router.Route;

@Route(TemplateView.ROUTE)
public class TemplateView extends Div {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the TemplateView and TemplateIT are equal should we have a common package with tests that would be used by both development tests and production tests?
see flow-tests/test-embedding/embedding-test-assets and flow-tests/test-embedding/embedding-test-assets for reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@caalador caalador self-requested a review November 8, 2021 06:35
Comment on lines 16 to 17
<module>embedding-test-assets</module>
<module>addon-with-templates</module>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<module>embedding-test-assets</module>
<module>addon-with-templates</module>
<module>addon-with-templates</module>
<module>vite-test-assets</module>

Copy paste name and wrong order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😣 , done

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 14 issues

  • MAJOR 11 major
  • MINOR 3 minor

Top 10 issues

  1. MAJOR LitTemplateParserImpl.java#L98: Reduce the total number of break and continue statements in this loop to use at most one. rule
  2. MAJOR NpmTemplateParser.java#L104: Reduce the total number of break and continue statements in this loop to use at most one. rule
  3. MAJOR PolymerTemplate.java#L102: Remove this call from a constructor to the overridable "initModel" method. rule
  4. MAJOR NodeTasks.java#L1: Class com.vaadin.flow.server.frontend.NodeTasks$Builder defines non-transient non-serializable instance field lookup rule
  5. MAJOR NodeTasks.java#L148: Make "lookup" transient or serializable. rule
  6. MAJOR TaskCopyTemplateFiles.java#L69: Exceptional return value of java.io.File.mkdirs() ignored in com.vaadin.flow.server.frontend.TaskCopyTemplateFiles.execute() rule
  7. MAJOR AbstractDevServerRunner.java#L114: Make "devServerStartFuture" transient or serializable. rule
  8. MAJOR AbstractDevServerRunner.java#L191: Invoke method(s) only conditionally. rule
  9. MAJOR AbstractDevServerRunner.java#L382: Either re-interrupt this method or rethrow the "InterruptedException". rule
  10. MAJOR AbstractDevServerRunner.java#L608: Null passed for non-null parameter of java.util.concurrent.CompletableFuture.getNow(Object) in com.vaadin.base.devserver.AbstractDevServerRunner.handleRequest(VaadinSession, VaadinRequest, VaadinResponse) rule

@joheriks
Copy link
Contributor Author

joheriks commented Nov 8, 2021

Bakery verified working with this PR snapshot:

Vite Webpack
Dev
Prod

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 22.0.0.beta2 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

4 participants