PB-285: Added dispatcher to store actions and improve startup procedure (language change)#644
Merged
PB-285: Added dispatcher to store actions and improve startup procedure (language change)#644
dispatcher to store actions and improve startup procedure (language change)#644Conversation
Passing run #669 ↗︎Details:
Review all test suite changes for PR #644 ↗︎ |
|||||||||||||||
9324d46 to
ed805f3
Compare
dispatcher to all store actions related to the URL parsingdispatcher to store actions and improve startup procedure (language change)
b55757b to
cb5601f
Compare
pakb
reviewed
Feb 20, 2024
Contributor
pakb
left a comment
There was a problem hiding this comment.
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
setSomethingthen the payload should be{ something: ..., dispatcher: ... } - Only use
valueas key, with the exception of places where it helps to now what kind of payload we are dealing with (such asaddLayer)
But a mix of both might be confusing in the long run.
src/modules/map/components/footer/backgroundSelector/BackgroundSelector.vue
Outdated
Show resolved
Hide resolved
cb5601f to
75e1dbf
Compare
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.
This is based on vuex docu.
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
6f52f40 to
71307b0
Compare
71307b0 to
67d6761
Compare
pakb
approved these changes
Feb 21, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order to improve startup procedure I added a global new
dispatcherattribute 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:
setLayersIt also solved the following bugs:
TODO in separate PR:
Test link