Skip to content

PB-285: Added dispatcher to store actions and improve startup procedure (language change)#644

Merged
pakb merged 12 commits intodevelopfrom
feat-PB-259-performance
Feb 21, 2024
Merged

PB-285: Added dispatcher to store actions and improve startup procedure (language change)#644
pakb merged 12 commits intodevelopfrom
feat-PB-259-performance

Conversation

@ltshb
Copy link
Contributor

@ltshb ltshb commented Feb 15, 2024

In order to improve startup procedure I added a global new dispatcher attribute to all store actions related to the URL query parameter. This should also be done for all actions, but for the time being only added it to the url parameter related actions.

This new generic parameter gives the possibility to do different actions in the
store plugins depending on the source of the event. This avoid bulky/flaky tests
using local closure flags.

This PR already improved some performances:

  • language changes performance improves as it don't re-add every layer one at a time, but in a bulk operation
  • Don't load the layer config and topics at startup twice if the language is not the browser default
  • Setting default topic layers is now about 400-500 ms faster due to bulk store mutation setLayers
  • improve logging for debugging by introducing a debug log for every mutation

It also solved the following bugs:

  • meteoschweiz and schneesport topic default layer reverse order (due to plConfig)
  • Fix topics that only specified selectedLayers in backend (BFS, IVS, KGS, ...)

TODO in separate PR:

Test link

@cypress
Copy link

cypress bot commented Feb 15, 2024

Passing run #669 ↗︎

0 176 21 0 Flakiness 0

Details:

PB-259: Small code improvement from review
Project: web-mapviewer Commit: 67d6761820
Status: Passed Duration: 03:51 💡
Started: Feb 21, 2024 3:04 PM Ended: Feb 21, 2024 3:08 PM

Review all test suite changes for PR #644 ↗︎

@ltshb ltshb force-pushed the feat-PB-259-performance branch 9 times, most recently from 9324d46 to ed805f3 Compare February 19, 2024 14:53
@ltshb ltshb changed the title PB-285: Added dispatcher to all store actions related to the URL parsing PB-285: Added dispatcher to store actions and improve startup procedure (language change) Feb 19, 2024
@ltshb ltshb requested a review from pakb February 19, 2024 15:19
@ltshb ltshb marked this pull request as ready for review February 19, 2024 15:19
@ltshb ltshb force-pushed the feat-PB-259-performance branch from b55757b to cb5601f Compare February 20, 2024 13:34
Copy link
Contributor

@pakb pakb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a general comment on this new way of key used for the payload, I think we should decide one of two possibilities :

  • Have some kind of naming convention for the value's key, for example if the action is called setSomething then the payload should be { something: ..., dispatcher: ... }
  • Only use value as key, with the exception of places where it helps to now what kind of payload we are dealing with (such as addLayer)

But a mix of both might be confusing in the long run.

ltshb added 11 commits February 21, 2024 15:54
When changing the topic we need to set clear the active layers and set a bunch
of new activated layers. This was done with several dispatch which was quite
inefficient.

Now we use only one dispatch with the new array, this way for the default topic
`ech` we gain about 500ms on development build.

Also fixed a bug in the infamous plConfig of topics (used by schneesport topic)
in which layers were added in reverse order.
Topics on the backend that only specified the selectedLayers without activatedLayers
were not added.
Due to previous commit that set the topic layers in one dispatch, we must make
sure that the layer list to set has no duplicate.
…rsing

This should also be done for all actions, but for the time being only added
it to the url parameter related actions.

This new generic parameter gives the possibility to do different actions in the
store plugins depending on the source of the event. This avoid bulky/flaky tests
using local closure flags.
During the startup we had several issues with the topic selections.

- the main issue is that we first loaded the default language and topic at startup
  and change them later by parsing the URL. One of the drawback was that the
  layerconfig and topics would have been loaded twice from the backend and
  that the default topics layer would have been set before the current topic
  layers resulting in a lot of store mutations for nothing.
  Now we use the lang and topic from the url if they are available at app startup.
  In order to do this as quickly as possible we now keep in store only the topic ID
  as current instead of a link to the current topic.
- the topic management plugin was a bit buggy and not clear with flag to do stuff
  in different mutation trigger. Now we make use of the dispatcher which make
  the code much clearer. In this plugin for example we only want to set the layers
  when the topic has been changed by the user and not by the URL parsing. Url
  parsing event should takes layer from the URL layers param.
This fixed some bugs of topics management introduced in later PR commits and
make the startup procedure clearer by clearly separated concerns.

Now the store plugins don't have any more special code for the startup. The
startup is by the application management router plugin which does the application
startup routing.

Also removed duplicated topics management events which slightly improve startup
time.
Vuex mutation are costly and sending in a loop a lots of event is not a good
idea. When changing the languages we need to reload the layer config and
reset all active layers in order to have them translated instead of doing this
in a loop we all set them in one event.
This improve greatly performance with long active layer list.
Instead of an arbitrary name for the dispatch value, uses a convention as follow:

- Use the same keyword as the one in the store
- Some exception for URL parameter which uses the url parameter for simplification
@ltshb ltshb force-pushed the feat-PB-259-performance branch from 6f52f40 to 71307b0 Compare February 21, 2024 14:54
@ltshb ltshb requested a review from pakb February 21, 2024 14:54
@ltshb ltshb force-pushed the feat-PB-259-performance branch from 71307b0 to 67d6761 Compare February 21, 2024 15:00
@pakb pakb merged commit fd81ad6 into develop Feb 21, 2024
@pakb pakb deleted the feat-PB-259-performance branch February 21, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants