Reduce addRoutes function calls

[grimasod]

Related issues

closes #3442

Short description and why it's useful

Greatly reduces the number of times the router beforeEach hooks are run

Screenshots of visual changes before/after (if there are any)

Which environment this relates to

Check your case. In case of any doubts please read about Release Cycle

• Test version (https://test.storefrontcloud.io) - this is a new feature or improvement for Vue Storefront. I've created branch from develop branch and want to merge it back to develop
• RC version (https://next.storefrontcloud.io) - this is a stabilisation fix for Release Candidate of Vue Storefront. I've created branch from release branch and want to merge it back to release
• Stable version (https://demo.storefrontcloud.io) - this is an important fix for current stable version. I've created branch from hotfix or master branch and want to merge it back to hotfix

Upgrade Notes and Changelog

• No upgrade steps required (100% backward compatibility and no breaking changes)
• I've updated the Upgrade notes and Changelog on how to port existing VS sites with this new feature

IMPORTANT NOTICE - Remember to update CHANGELOG.md with description of your change

Contribution and currently important rules acceptance

• I read and followed contribution rules
• I read the TypeScript Action Plan and adjusted my PR according to it
• I read about Vue Storefront Modules and I am aware that every new feature should be a module

[pkarw]

Hi there! Good point @grimasod - please check the #3412 which is partially solving this issue but just for the route dispatcher using locking mechanism on the RouterManager level; probably won’t work for the case you have just fixed but just to be sure we’re not duplicating the code for the same issue

[pkarw]

I’m not sure if it is reducing the router.addRoutes to just single one (which can obviously solve the multiple routes call once and for all) but maybe we can do it by postponing the registration till very end just before app.mount call(?) the other option is to add a paragraph to the docs on how to use the locking mechanism in the RouteManager level and a note to not use the router hooks directly

Or even better - add docs + add a mechanism to register hooks from roterManager level that are locking the calls themselves

Then, we need to find all the instances or adding the hooks listeners to router directly and replace them with the wrapper calls from the router manager

I think this makes sense as the routermanager is the only class which can be safely used for adding the routes because of the url dispatcher module

@filrak ive mentioned you in the other PR regarding passing routermanager instead of router to the module constructors
@patzick wdyt?

[ResuBaka]

This looks nice.

It reduces one of the guards like from 14 times to only once in my testing and all URLs are working like they should.

The only nice thing would be to have some unit tests for these changes.

[grimasod]

Hi @pkarw, yeah I saw #3412 and tested after it was merged. It didn't affect this issue.

As you suggest, postponing all additions to the end could ensure there was exactly only one addRoutes. I thought about something like that when I started working on this, but it seemed quite complex! And I wonder if it could affect some custom modules or themes?

So, I thought the solution in this PR might be more suitable for a hotfix, as it shouldn't have any knock-on effects. It's not 100% perfect, but it's a huge improvement. In our custom theme, it reduced User beforeEach calls from about 750 to just 1 or 2, depending on the page.

There's another point to consider (not dealt with in this fix) ...

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]

@ResuBaka Good point. I haven't done any unit testing work on VS, do you have any suggestions or pointers?

[ResuBaka]

@grimasod One point would be this test file which we have four the multistore.ts.

Then all unit test are under test/unit/<function/file you want to test>.spec.ts inside the folder the to testing file is.

Example of the structure:
core/lib/multistore.ts
core/lib/test/unit/multistore.spec.ts or core/lib/test/unit/setupMultistoreRoutes.spec.ts

Then the unit test are written in jest (docs) when you need some help you can write me in the slack a dm.

Test can be startet with yarn test:unit the watch mode of jest does currently not work in 1.10 I have only fixed it in 1.11. (When you want to use the watch feature for easier test writing you could fix it locally for you when you look at these PRs #3354, #3351)

@pkarw @patzick should we maybe backport my fix so more people can use the watch feature of jest in the older version?

[grimasod]

@ResuBaka Thanks. I'll look into it!

[grimasod]

Is it necessary to create routes for all stores?

Thinking about this more... isn't it actually incorrect to generate routes for all stores based on the routes of one store? For example:

• Each store can have a different theme, which could contain different pages
• Each store uses a different Elasticsearch index, which can contain different categories and products - this is quite common when selling overseas. Some products can't be shipped to certain countries.

[ResuBaka]

@grimasod

When I think about that we should do, for a quick improvement to your code check if the stores have enabled the appendStoreCode config value because when they don't they most likely have different/need to domains for each store so only mapping stores where the appendStoreCode is set to true would improve this too.

[grimasod]

@ResuBaka Yeah, you're right.

Static routes are added using localizedRoute which accounts for that config value, but the Dynamic routes function has its own construct. It should probably use localizedRoute, if possible.

[patzick]

This looks really good, I just need a while to test this properly first to be sure :)

[patzick]

I've checked it and i think it works nice and fast. @pkarw, would you mind to re-review?

[pkarw]

Thanks for the discussion, it was really cool! Great insights 👍

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

@grimasod how about trying to implement these upgrades?

[grimasod]

Thanks for the feedback :)

@pkarw I can work on it, but it won't be soon. I'm releasing our v1.10 site this week and don't have much time for anything else.

1. This sounds good. I just need to look into a bit more, to understand where to call flushRouteQueue

2. This seems like would need quite a bit of testing and maybe it's too risky for a hotfix. WDYT about doing this part in a later PR for v1.11?

[pkarw]

Sure, 1.11 sounds reasonable @patzick

[pkarw]

Cool; i created a #3454 just for that

[mryellow]

Just stopping by to thank you guys for saving me many more hours of debugging on an unrelated project. Also to add a few observations and keywords for those coming after.

Interestingly this only happens with async calls to next() in beforeEach navigation guards when addRoute is used after beforeEach.

router.beforeEach((to, from, next) => {
  // Single call
  //next();

  // Many calls
  //setTimeout(() => next(), 5000);

  // Many calls
  Promise.resolve()
    .then(() => next());
});

routes.forEach(route => router.addRoute(route));

Also note that this does not happen in a very simply Vue application such as this fiddle below. It's somehow related to further complexities around how we're bootstrapping up Vue.

https://jsfiddle.net/MrYellow/zps18dg0/