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

Optimize the RouterManager.addRoutes #3454

Closed
pkarw opened this issue Sep 2, 2019 · 9 comments
Closed

Optimize the RouterManager.addRoutes #3454

pkarw opened this issue Sep 2, 2019 · 9 comments
Labels
3: Medium complexity feature request Requests for new features. Please be as specific as possible and provide proposal API if it you can P3: Normal Priority mark - normal priority
Milestone

Comments

@pkarw
Copy link
Collaborator

pkarw commented Sep 2, 2019

What is the motivation for adding / enhancing this feature?

Related to: #3443

The goal: to reduce the number of times the router beforeEach hooks are run

  1. @grimasod to optimize this route registration, going down to one and only router.addRoutes call I belive we need just to implement a RouterManager.routeQueue[] where RouteManager.addRoutes Has new optional argument useRouteQueue=False calls are adding the routes and then the method RouteManager.registerRoutes or RouteManager.flushRouteQueue which is doing the real registration; this method should be called after all the theme and module routes were registered. We need to change all the core calls to Router.addRoutes to set the useRouteQueue=true. The default value would be `false for backward compatibility + the calls made after the queue was flushed

  2. @grimasod you’re probably right about the multistore route registration but probably: a) just for CSR (as you said for SEO and language switching features we need to have the server side links for other lang versions); b) based on the storeCode - we always know the current store code because it’s being guessed on URL/Header/Host before modules were registered. So knowing the currentStoreView we can register only the right set of routes (if they’re different from the default routes) - so for default there would be single route registration, for any other lang version probably 2 calls but not 3,4,10 ...

@pkarw pkarw added the feature request Requests for new features. Please be as specific as possible and provide proposal API if it you can label Sep 2, 2019
@pkarw pkarw added this to the 1.11.0-rc.1 milestone Sep 2, 2019
@pkarw pkarw added 3: Medium complexity P3: Normal Priority mark - normal priority labels Sep 2, 2019
@pkarw
Copy link
Collaborator Author

pkarw commented Sep 5, 2019

Additional comment for point 2:

Is it necessary to create routes for all stores?

Considering it's not possible to navigate directly between stores. Instead, the app gets reloaded, and all the routes recreated. So only the current store routes need to be added. Right? The exception is SSR for SEO, but that can be handled separately. This also seems like a big change though and could possibly affect some customizations.

In the end, I thought this simple fix would be best for 1.10.1 and maybe a bigger update can be planned for 1.11. What do you think?

@grimasod
Copy link
Contributor

grimasod commented Sep 6, 2019

@pkarw
Thanks for copying the comments to this issue. I'll try to get to work on it soon.

for default there would be single route registration, for any other lang version probably 2 calls

Do you think the default store routes are needed when on another store?

@pkarw
Copy link
Collaborator Author

pkarw commented Sep 6, 2019

I think they're not - probably - hard for me to find the exceptions ...

@pkarw
Copy link
Collaborator Author

pkarw commented Sep 6, 2019

@grimasod it would be coll if by chance of this PR we'll add a priority field to routes so modules/theme routes can overlap more clever way :) If we have a queue it won't be any problem.

Related to: https://vuestorefront.slack.com/archives/C8JMCHQ6S/p1567757048024000

@grimasod
Copy link
Contributor

grimasod commented Sep 6, 2019

@pkarw Sure :)
I wonder if theme routes should always take priority? Or by default at least

@pkarw
Copy link
Collaborator Author

pkarw commented Sep 6, 2019

by ddefault theme should have the highest priority I belive

@pkarw
Copy link
Collaborator Author

pkarw commented Sep 10, 2019

@grimasod I just wanted to check the status as we're planning to release 1.11rc1 on 19th of September

@grimasod
Copy link
Contributor

@pkarw I've started on part 1, but it's not so simple - for example, Dynamic Route processing expects the routes to be already registered. I have limited time to work on this, but I hope to make some progress by the end of this week

@grimasod
Copy link
Contributor

@pkarw I've made a PR for part 1, but it's not finished. There are issues with dynamic routes

@grimasod grimasod mentioned this issue Sep 17, 2019
6 tasks
@pkarw pkarw closed this as completed Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Medium complexity feature request Requests for new features. Please be as specific as possible and provide proposal API if it you can P3: Normal Priority mark - normal priority
Projects
None yet
Development

No branches or pull requests

2 participants