Polymer / pwa-starter-kit

Starter templates for building full-featured Progressive Web Apps from web components.
https://pwa-starter-kit.polymer-project.org
2.36k stars 431 forks source link

replace the router from pwa-helpers with Vaadin.Router #195

Closed vlukashov closed 5 years ago

vlukashov commented 6 years ago

Vaadin.Router is a small and powerful router for Web Components. It enables a full set of features needed to create app-like experiences on the web: lazy loading, animated transitions, navigation guards, parent layouts, redirects and more. It is framework-agnostic and works equally well with all Web Components, so it would be a good fit for the PWA starter kit.

Please let me know what do you think of having a more powerful router in the baseline PWA starter.


Lighthouse score impact: under 1 point.

pwa-starter-kit-comparison lighthouse

Lighthouse audit report (master) Lighthouse audit report (vaadin-router)

The audits were run locally against the es6-bundled build (that's the default for Lighthouse).

It comes as a bonus (not reflected in the Lighthouse score) that all routing code is loaded lazily and is not blocking the initialization of the app shell. That is, the app shell is loaded and rendered first, then the routing code gets loaded and executed.

hastebrot commented 6 years ago

I haven't investigated what vaadin-router exactly does, but I'd prefer a starter kit that does not send any usage statistics (in both development and production mode).

"dependencies": {
   "@vaadin/vaadin-usage-statistics": "^2.0.0-alpha4",

via: https://github.com/vaadin/vaadin-router/blob/master/package.json

(Nevertheless, vaadin-router seems to be a nice library)

jsilvermist commented 6 years ago

I rather like the pwa-helpers router, it's simple, fast, and easy to build upon.

notwaldorf commented 6 years ago

In particular, we explicitly wanted a router that doesn’t blow away all the views you have constructed. If a page is expensive to construct, you’re going to pay that cost over and over anytime you switch to it, and it seems that this router does that by default :(

abdonrd commented 6 years ago

I like the Vaadin.Router, it seems more simple!

But waiting https://github.com/vaadin/vaadin-router/issues/44 for the reasons that @notwaldorf comments.

vlukashov commented 6 years ago

@hastebrot, the concerns about stats gathering are understandable. I should mention here that it gets activated for a small random sample, it is anonymous, and there is a way to opt-out. However, I am glad to see these concerns being voiced as the first comment on this PR, because this PR is as much a feedback channel as an enhancement proposal.

Let's focus on the features this enhancement adds to the starter kit.

vlukashov commented 6 years ago

@jsilvermist, simplicity is great but as a starting point it works best when building one of a kind apps with expert teams. For a starter kit that would be used to start lots of apps and targets basic cases, it might be great to have common features already implemented.

To me it looks that it's a matter of choice, and both options have their trade-offs. Thanks for the feedback, though! Please, keep it coming.

vlukashov commented 6 years ago

@notwaldorf, if you had 3 new features or changes to wish for in Vaadin.Router, what would be the two other?

Why do you see view caching as critical for the PWA starter kit? It is as important to remove stale view elements from the DOM to avoid resource leakage. That's what the current router does and that's what this PR removes. It is not ideal in some use cases, but it is arguably a better default for a generic starter.

notwaldorf commented 6 years ago

@vlukashov that's a very hard question, because i wouldn't use a different router -- i like to keep my routing strategy incredibly simple and transparent, so that there's no question about what initiated a route change. This is exactly why we picked the pwa-helpers router. You can use a different routing strategy for your application in the same way you can use different UI elements; however, in the interest of keeping the pwa-starter-kit template as simple as possible, we would prefer not to change to a different router.

Constructing views and custom elements is by definition not free, especially for resource intesive pages, so generally doing away with an entire view on navigation is bad. Again, you can change this in your app, but in the interest of keeping the template with best practices (especially for beginners), this is the approach we thought best.

abdonrd commented 5 years ago

@vlukashov I just tried to use this.

With npm start (polymer serve) works well, but if I build and serve the project (npm run build && npm run serve) it doesn't work. 😕

Empty main:

screenshot 2019-01-24 at 15 27 23

New branch with the last version: https://github.com/Polymer/pwa-starter-kit/compare/master...abdonrd:vaadin-router

abdonrd commented 5 years ago

Fixed! https://github.com/Polymer/pwa-starter-kit/compare/master...abdonrd:vaadin-router