Separate components unintentionally updating one another
I don't know how I ended up here, but I have a bizarre bug where I have two windows open and one of them sends data to another. There is no code that shares state between the two.
37 Replies
https://github.com/MrVauxs/pf2e-graphics/blob/main/src/view/ItemAnimations/tabs/animation-config.svelte
https://github.com/MrVauxs/pf2e-graphics/blob/main/src/view/ActorAnimations/submenus/actor-animations.svelte
Item Animations sends data to ActorAnimations but not vice versa, nor did I build it to do that
Hard to say from the two files posted.
There could be problems with the writable derived usage. I don't actually use
writableDerived
and my custom stores use the simpler propertyStore
helper which itself wraps writableDerived
. So offhand without looking into things more I can't tell immediately if you have a problem there. It looks like that code is duplicated across files. Perhaps move it into it's own standalone utility function.
The problem could be elsewhere. Offhand though probably not likely is reusing a single TJSDocument instance where the writable derived store is targeting the same TJSDocument instance across apps. I'm only mentioning that because the instance is a prop.
Perhaps add console log statements into the writable derived code near setFlag
and log the ID of the document being set to get a bit more insight into what is occurring.I have tested the same thing with your TJSDocumentProperty and getting the same results. What is weird is that one menu doesnt experience that issue while the rest do, even one thats not actually attached to a TJSDocument, but a setting.
UserAnimationsShell
has the exact same structure and yet isn't affected
which is the strangest part for me
Logging what documents are updated doesnt really help, I need to know what actually causes ItemAnimations
to, somehow, send an update to ActorAnimations
and make it update itself with that data
And this only happens when the window is opened, so its definitely in the .svelte
files themselvesSo... I'd like to see how TJSDocument is handled presumably in the outer JS code. TJSDocument is leaky presently where the underlying handling doesn't manage registering with the Foundry doc efficiently. Manually invoking
destroy
on the TJSDocument instance might come into play when apps open / close.
This will be fixed in 0.2.0
as TJSDocument will only associate with the underlying Foundry doc when there are actual subscribers to the TJSDocument instance.There isn't any outer code besides the creation of the window
i.e.
Button on Sheet passes
Document
to SvelteApplication -> SvelteApplication calls AppShell
with the Document
as a prop
AppShell
turns the prop (Document
) into TJSDocument
It then passes that TJSDocument
to any child components, including adding new animations.Add an
onDestroy
handler where the TJSDocument instance is created. Something like:
onDestroy(() => doc.destroy());
Just trying to rule out a deficiency / potential edge case here. As mentioned this will be resolved in 0.2.0
for more automatic handling.No luck
Not that it would work I think, the issue is when these subscription still exist
Yeah.. Probably not it, but good to keep in mind that the current TJSDocument is a bit weak in how it manages the connection to the underlying Foundry doc. It's always a good idea to manually destroy a TJSDocument instance when it goes out of scope for now.
would passing them as context instead of props do anything? 🤔
Just make the code cleaner with less
prop drilling
. You could have the TJSDocument instance in the outer SvelteApplication / JS code and invoke destroy on it when the app is closed for instance.That was my thought but then
context
is supposed to only work on the component it was created on and its descendantsHere is my take on making a standalone writableDerived function; do modify it accordingly:
Given the issue is that the TJSDocument is somehow sharing one-way updates to other unrelated components, maybe using contexts would get rid of the issue by enforcing that limit?
Making sure that the TJSDocument instance is unique between both apps and making sure
destroy
is invoked on it should be the important part. Props vs context is more for code cleanliness.
Also just a head check to make sure the actor and item are top level documents and say the item is not somehow and embedded item. Not saying that is the problem / more of a curiousity / edge case.The item can be part of the same actor, but the bug also happens on
WorldAnimations
and that doesnt use TJSDocument at all
Honestly just makes me ask what I did wrong when creating these windows because this seems like a basic issue to have in something like a system, where you can have multiple items open and you obviously dont want them to share state.It's certainly possible that the problem lies outside of the Svelte components. Divide and conquer is the standard approach to take. Perhaps in the Svelte components for the actor / item variations explicitly retrieve an actor and item doc ala
game.actors.get('<ID>')
and game.items.get('<ID>')
and see if there is sharing of data still. IE remove the external launching of the code and try to determine if the sharing lies with the TJSDocument / writable derived code.Made a test window and I managed to replicate this behavior once, but it then went away 😕
O
Alright, I think I got a minimum repro, though it only happens on the first update
Any subsequent update works normally
aaaand it went away again
Well I appreciate your gumption as any info you glean will likely help out a
0.2.0
store based approach to get just a bit more out of TJSDocument in the interim before the Svelte 5 transition.Rewriting the whole approach, so far I don't see this bug returning
Probably some weird interaction from my weird code. Now it's much nicer.
You can never rule out that six inches between ones ears! ;P Keep me posted. I found it curious that you saw similar issues between the demo code I posted and your effort.
Finished the 2.0, it works perfectly lol
GitHub
Windows 2.0 by MrVauxs · Pull Request #41 · MrVauxs/pf2e-graphics
Fixes #38
More generalized approach, more reliance on the svelte magic than writing your own stores.
Also comes with a JSONEditor.
I think the most meaningful change is the actual TJSDocument is now inside the AnimationEditor
Which is shared across every window, as opposed to having the window making the TJSDoc and passing it as a prop
Just to point out the description above indicates a solution to the lapsed / dangling listener problem. As mentioned
TJSDocument
isn't as robust as it will be in 0.2.0
, so I suspect that the issue you encountered likely is due to not manually destroying / cleaning up TJSDocument
between app instances. I also haven't reviewed the whole scope of your code.
When you switch wrapped core documents with a single instance of TJSDocument
it will clean up the old associations with the core document when the switch is done.
None of this is particularly a problem with TRL, but an age old anti-pattern common in all OOP languages / observer pattern. The nice thing though is that 0.2.0
will make this process a bit easier to get right.
The improvement for 0.2.0
is that TJSDocument
will only associate with the wrapped underlying core document when there are active subscribers to TJSDocument
. If the subscriber count to TJSDocument
drops to 0 then TJSDocument
automatically unsubscribes to the core document. It will re-associate with the underlying core document if the subscriber count increases.
With the current TJSDocument
implementation there are two levels of indirection. 0.2.0
will make that one level of indirection where you just have to be concerned about managing the subsriber count directly with TJSDocument
. When used in a Svelte component context this happens automatically.Feeling like printing this message out on paper and crushing it, its happening again :coolCry:
Well.. I'd be glad to take a look at the complete source code. You mentioned creating a small standalone instance of the problem right? Perhaps I can start there and take a look at causes then try it with the currently released TRL version and the improved TJSDocument support in the unreleased
0.2.0
version. If it goes away by just swapping that out then that is progress.I will try to replicate it but even as of now this is becoming tedious to find
Depending on the type of issue even reviewing the browser retained memory between snapshots in the dev tools can help identifiy a leak type problem which can occur with lapsed listeners. This is my assumption on what is going on though.
I can no longer replicate the issue, however, instead I got a new one, the world settings aint getting updated
grr
Well I replaced
storeSettings.getWritableStore
with my own writable that on change just updates the setting. Will break with multiple GMs accessing the menu at once but beyond that...
Not sure what is happening on the backend that TJSSettings is not saving the changes. I assume its to do with me storing an object of keyed arrays so the changes to the arrays are moot.Ahhh... Yeah... This is something separate from the other TJSDocument issue.
In the
0.2.0
TJSGameSettings you can set an object and it won't check / verify for equality. Alas the original TJSGameSettings code has a check which causes the object test to fail when setting an object. This has been long removed since Oct / Nov '23, but just hasn't been released.😩
FWIW both of these are in the same code
and they do interact thru the titular bug (one window updates, somehow sometime the other window has that update too)
Well.. I'll do my best to wrap up current work and get an initial
0.2.0-next.1
/ beta releases out. There will still be some rough areas in the standard library I'll be tweaking, but it's pretty important to get some of those long term committed fixes out.I'd gladly test it given the module is still in its relative infancy
Backend is pretty much done, the only thing that gives me perpetual torment is the UI
If it means that torment will be used for beta-testing so be it
I need to wait for the animations library to get bigger anyway, I can't match my own autorec that quickly
This upcoming ~8 days is relatively free on my schedule, so I'm going to be smashing through as much as possible.