Pins: Consider external updates and allow ignoring change sources#768
Pins: Consider external updates and allow ignoring change sources#768oeninghe-dataport wants to merge 9 commits into
Conversation
As we use the vite plugin enriched console, we do not specify the source of a log message manually.
This ignores changes originated from addressSearch to the pins coordinate for the reverseGeocoder in the snowbox. Fixes #655
oeninghe-dataport
requested review from
czirkelbach,
dopenguin,
raschju and
warm-coolguy
as code owners
|
The target should probably be |
Fixed. 
|
| ) | ||
| return | ||
| } | ||
|
|
||
| watcherConfig.handle = watch( | ||
| () => store[watcherConfig.source.key], | ||
| watcherConfig.callback, | ||
| (value) => { | ||
| const sourceKey = watcherConfig.source.key + 'Source' |
There was a problem hiding this comment.
This lacks documentation. Please add information e.g. to the TSDoc of the composable that, if I watch the parameter coordinate I will ignore updates if I have a parameter coordinateSource in my store and have ignoredSources configured.
As this requires a lot of knowledge and extra implementation for each plugin with a possible reference loop, isn't there a possibility of a different approach without the need of further boilerplate in any plugins?
There was a problem hiding this comment.
For the documentation: dfb50f2
On your question:
I'm also not too happy with the current solution. However, the following aspects convinced me to go with this:
- The pins plugin seems to be the natural participant in reference loops, even in the future. It allows reading and writing of other plugins in a useful way, which is the case for few plugins; usually, our plugins communicate in one direction only. I don't expect that we will need this in another plugin in the future.
- I found no approach without further boilerplate (that is not much more "hack-y" than this one). At least this approach allows re-using it in every plugin that refers to pins.
There was a problem hiding this comment.
I've fiddled around a bit with the help of Copilot and came up with a832128. Take a look and let me know if you find any flaws.
There was a problem hiding this comment.
- I do not think this idea works (yet, or at all). I added a
console.logto theif(...)statement in line 138 ofuseStoreWatcher, and this is not executed. I'd expect that this line is executed in the case of AddressSearch, Pins, ReverseGeocoder: Reference hell #655. - I'm not quite sure I understood the code perfectly yet. If we'd want to introduce this, this needs more documentation.
- If we'd want to introduce this, IMO, we need unit tests here. I know that we decided in general to write tests in a later stage, however, this mechanism is hard to test manually and prone to errors and regressions, and I'd make an exception here.
There was a problem hiding this comment.
Doesn't work yet
Oh, it does work.
Previously, it would further resolve to Billstraße 144. The loop is broken.
Documentation
There are several TSDoc and normal comments. Can you elaborate on what exactly needs documenting?
Tests
Sure, added two for the loop-breaking-mechanism e6571d9
|
3 participants
Summary
Consider external updates (#766) and allow ignoring change sources (#655).
Instructions for local reproduction and review
See #766 and #655
Relevant tickets, issues, et cetera
Fixes #766
Fixes #655