Skip to content
This repository has been archived by the owner. It is now read-only.

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

Closed
wants to merge 2 commits into from

Conversation

@vlukashov
Copy link

@vlukashov vlukashov commented Jul 20, 2018

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.

vlukashov added 2 commits Jul 10, 2018
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.

See https://github.com/vaadin/vaadin-router for details.
@hastebrot
Copy link

@hastebrot hastebrot commented Jul 20, 2018

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
Copy link
Contributor

@jsilvermist jsilvermist commented Jul 20, 2018

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

@notwaldorf
Copy link
Member

@notwaldorf notwaldorf commented Jul 20, 2018

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
Copy link
Contributor

@abdonrd abdonrd commented Jul 20, 2018

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

But waiting vaadin/vaadin-router#44 for the reasons that @notwaldorf comments.

@vlukashov
Copy link
Author

@vlukashov vlukashov commented Jul 20, 2018

@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
Copy link
Author

@vlukashov vlukashov commented Jul 20, 2018

@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
Copy link
Author

@vlukashov vlukashov commented Jul 20, 2018

@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
Copy link
Member

@notwaldorf notwaldorf commented Jul 20, 2018

@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.

@notwaldorf notwaldorf removed their request for review Aug 30, 2018
@keanulee keanulee closed this Dec 17, 2018
@abdonrd
Copy link
Contributor

@abdonrd abdonrd commented Jan 24, 2019

@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: master...abdonrd:vaadin-router

@abdonrd
Copy link
Contributor

@abdonrd abdonrd commented Jan 25, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants