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

Support lazy loading dependencies for views #5537

Closed
9 tasks
manolo opened this issue Apr 23, 2019 · 20 comments
Closed
9 tasks

Support lazy loading dependencies for views #5537

manolo opened this issue Apr 23, 2019 · 20 comments

Comments

@manolo
Copy link
Member

manolo commented Apr 23, 2019

As described in #5339 we should be able to produce chunks per each view.

Acceptance criteria

  • Each route should produce a different frontend/my-view.js file
  • User should be able to merge multiple views in one chunk
  • User should be able to disable chunking
  • webpack frontend/generated-flow-imports.js should import all those fragments.
  • theme stuff should be in the frontend/generated-flow-imports.js file
  • frontend/generated-flow-imports.js should not load any chunk, but have a function reference so as server can do it
  • previously to visit a new view, it's corresponding chunk should be loaded
  • Documentation about customizing chunks should be updated
  • Validate implementation in bakery
@Legioth
Copy link
Member

Legioth commented Apr 25, 2019

I would like to suggest one addition, unless it's already implemented to deal with #5509: All @JsImport annotations that are on the classpath but not reachable through any @Chunk should be included in a special fallback chunk, which is automatically loaded if that @JsImport is actually encountered.

What this means in practice is that any omissions wouldn't cause a broken application, but only that the end user has to download an additional chunk that might be larger than necessary.

@manolo
Copy link
Member Author

manolo commented Jul 1, 2019

Technical Considerations:

@mehdi-vaadin
Copy link
Contributor

mehdi-vaadin commented Jul 3, 2019

#5993 and #5658 should be considered here.

@manolo
Copy link
Member Author

manolo commented Jul 3, 2019

Considerations about issues that the implementation should cover related with missing imports:
1.- Scanner could be adjusted so as it might consider occurrences of certain method calls like dynamic routes (can be handled in separated task-PR)
2.- We might produce a chunk-extra.js with all @JsModule in classpath not visited, it will be loaded by the server when visiting a component not visited yet
3.- We might add some attribute to the @Chunk for scanning extra classes for that fragment

[1] cannot cover all uses cases because of injection, reflection, etc cases
[2] covers all cases and does not break, but forces loading of extra components that will never be used in the app.
[3] is the most versatile, and allow the user to select which routes have an extra component

@pleku
Copy link
Contributor

pleku commented Apr 17, 2020

This probably will be affected by using Webpack 5 #6773 especially if we choose to support module federation feature.

@Legioth
Copy link
Member

Legioth commented Apr 17, 2020

What would that impact be? My understanding is that code splitting isn't directly tied to anything in Webpack except the fact that dynamic import() leads to a separately loaded chunk.

@ScriptedAlchemy
Copy link

If you need any info on module federation, let me know :)

@mstahv
Copy link
Member

mstahv commented Jun 2, 2020

Thanks @ScriptedAlchemy! Our team is currently busy on other topics, but we should probably get back to the front-end build soon, hopefully during the summer, as that is an area we get a lot of complaints about. We also hope that Webpack 5 would make the build faster for Vaadin developers.

@caalador
Copy link
Contributor

caalador commented Sep 17, 2020

Hi @ScriptedAlchemy

While trying to get the ModuleFederation to work I've stumbled upon the case that when I have 2 module federations with exposed components where both have and reference to polymer elements e.g. dom-module, dom-bind the application will fail as the bundles do not recognize that one of them already has initialized for instance dom-module and the other on then tries to redo the customElements.define

I haven found out how to get all the modules to understand the polymer share.

I have a sample project at
https://github.com/vaadin/skeleton-starter-flow/tree/proto/failingFederation/VaadinComponents
where you can note that a dom-x module will try to be defined twice from different sources.

If you have any hints on what should be done so that the polymer elements would be actually shared it would be much appreciated.

@ScriptedAlchemy
Copy link

Check my examples repo. You might not be sharing with semver so webpack doesn't know to use a singleton

@StefanCov
Copy link

Hi, I this the same as fragmenting?

If so, I would be interested in seeing this added to the latest version of Vaadin 14. Before the upgrade to Vaadin 14 we fragmented the views to make our login view much smaller. Now this will load the whole project on the initial load of the login view. It seems like a step back, upgrading Vaadin to run better but it makes our application slower on the initial load.

@Artur-
Copy link
Member

Artur- commented Apr 17, 2023

Let's merge some info from here and there into this issue. Things have changed a bit in the last years so here's what I think should be done now:

  1. Add a loadMode attribute to @Route so you can defined LoadMode.EAGER or LoadMode.LAZY
  2. Unless overridden, make the route for "" and "login" eager and the rest lazy by default
  3. Produce a separate JS file per route during the build. Import them either eagerly or using a dynamic import, depending on the defined LoadMode
  4. Include both CSS and JS in the per-route JS file
  5. Trigger loading of the route chunk when opening the view the first time

@knoobie
Copy link
Contributor

knoobie commented Apr 17, 2023

I would suggest to make LoadMode.EAGER the default and people have to opt-in for this especially with views that have large external npm dependencies. Doing it by default for every view, would also allow attacks to get information what views are present, if they find a way to traverse the css files, even tho the view's / routes are protected.

@Artur-
Copy link
Member

Artur- commented Apr 17, 2023

It would have to use a sha hash or similar as the key so it does not leak routing information

@Artur-
Copy link
Member

Artur- commented Apr 17, 2023

The reason for having "" and "login" as eager by default with the rest lazy is to be able to optimize most applications without any changes to the existing app code.

It is likely that the exact algorithm of building chunks per route need some tuning after testing also because creating a lot of small chunks is of no benefit, then it is better to merge everything or even eagerly load the components.

@knoobie
Copy link
Contributor

knoobie commented Apr 17, 2023

Your example would be the perfect reason why I would go with the opposite default: people that need this feature could easily add it and default application where the bundle is kinda small (excluding charts and stuff) would not really get much benefit from it and therefore are optimized / de-optimized for "nothing".

@Artur-
Copy link
Member

Artur- commented Apr 27, 2023

I am curious when you say "people that need this feature". The opposite of this feature is to load all dependencies of a given route upfront even if the view is never visited. Who needs that ever?

@knoobie
Copy link
Contributor

knoobie commented Apr 27, 2023

If you use just "stock Vaadin components", I would argue that most of the components are so lightweight that it doesn't matter and people that wanna include external / Npm based components via Annotation on Views should think about it / know about the possibility to change it to load lazily.

@Legioth
Copy link
Member

Legioth commented May 2, 2023

All else being equal, a smaller download size is better than a bigger download size. It doesn't if the difference is sometimes small if there are no obvious drawbacks from splitting. Are we aware of any such drawbacks?

@Artur- Artur- changed the title Implement @Chunk Support lazy loading dependencies for views May 8, 2023
@Artur-
Copy link
Member

Artur- commented May 8, 2023

This is now implemented as follows:

  • One JS file is created per @Route instead of just a single global one
  • The dependencies for the view are defined in that JS file
  • The JS file is lazily imported when the route class is used the first time
  • The dependencies for routes "" and "login" are by default loaded immediately instead of when first used
  • You can alter the default behavior by adding @LoadDependenciesOnStartup on your AppShellConfigurator class:
    • Only defining @LoadDependenciesOnStartup reverts to the old behavior of loading all dependencies immediately
    • @LoadDependenciesOnStartup(SomeView.class, OtherView.class) will load the dependencies of SomeView and OtherView immediately, instead of the views mapped to "" or "login"

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

No branches or pull requests

10 participants