TyphonJS•2y ago

TJSDocument and Token Issues

const tokens = cardData.targets
        .map(t => [t, fromUuidSync(t)])
        .filter(([_, t]) => t);
    const targets = new Map(
        tokens.map(([id, token]) => [id, new TJSDocument(token)])
    );

    const reducer = new DynMapReducer(targets);

const tokens = cardData.targets
        .map(t => [t, fromUuidSync(t)])
        .filter(([_, t]) => t);
    const targets = new Map(
        tokens.map(([id, token]) => [id, new TJSDocument(token)])
    );

    const reducer = new DynMapReducer(targets);

I have this piece of code that creates a reducer for token documents. This worked completly fine in v10 with no issues whatsoever but in v11 if ends up causing token movemnt issues and throws a few errors. I've not been able to pin point what part of TJSDocument ends up causing this issue. But replacing the above code with the following fixes it.

const tokens = cardData.targets
        .map(t => [t, fromUuidSync(t)])
        .filter(([_, t]) => t);

    const targets = new Map(
        tokens.map(([id, token]) => [id, new TJSDocument(token.actor)])
    );

    const reducer = new DynMapReducer(targets);

const tokens = cardData.targets
        .map(t => [t, fromUuidSync(t)])
        .filter(([_, t]) => t);

    const targets = new Map(
        tokens.map(([id, token]) => [id, new TJSDocument(token.actor)])
    );

    const reducer = new DynMapReducer(targets);

Ofc this fix isn't ideal and leads to complications but those aren't TJS based.

71 Replies

TyphonJS (Michael)•2y ago

Do tell.. I'm working a bit on TJSDocument right now, so good time to fix anything.

Nekro DarkmoonOP•2y ago

its hard to write the post in create mode so just gonna edit it in a sec https://media.discordapp.net/attachments/1121656000584355980/1121681836502950008/tZSGU3Eu2o.gif Getting a video from my live build with the error

TyphonJS (Michael)•2y ago

So TJSDocument hasn't changed between v10 & v11 at this point. It very well could be something specific to v11 changed under the hood that perhaps isn't well known / understood.

Nekro DarkmoonOP•2y ago

that's what I'm guessing too Here's a more detailed video

Nekro DarkmoonOP•2y ago

The card display itself is what stores the TJSDocuments after removing the cards and doing a full refresh the issue is resovled and it only affects tokens that have that a TJSDocument on a card

TyphonJS (Michael)•2y ago

I'm shooting for early next week for the next TRL release, so if there is anything that you can debug before then that is ideal. I can't really tell what the issue might be as I haven't attempted to use TJSDocument in the way that you are and the stack trace produced is all v11 / core, etc.

Nekro DarkmoonOP•2y ago

yup I'll do some mroe debugging to see what the issue might be

TyphonJS (Michael)•2y ago

One thing that I did notice is that you can probably update your Vite config to whatever is the latest in essential-svelte-esm as there were some small tweaks that get rid of the style.css error when using the dev mode. Particularly this line in the server section gets rid of it via forwarding it on to the main server:

[`^(/${s_PACKAGE_ID}/(assets|lang|packs|style.css))`]: 'http://localhost:30000',

[`^(/${s_PACKAGE_ID}/(assets|lang|packs|style.css))`]: 'http://localhost:30000',

Nekro DarkmoonOP•2y ago

ah thanks !

TyphonJS (Michael)•2y ago

Hopefully before the end of the year I'll have a CLI going again which will make the default configuration painless to use / much easier and automatically manage the config.

Nekro DarkmoonOP•2y ago

Nekro DarkmoonOP•2y ago

So I think it's trying to update this config right here but it ends up seeing a TJS applicaiton

TyphonJS (Michael)•2y ago

Ahh... There we go... The app.closing getter must be new in v11. There really is no great solution to get event callbacks w/ the core document model, so TJSDocument fakes the minimum API necessary to appear to be an app. So w/ that property missing in TJSDocument that will trigger as !(undefined) is true. A quick fix is to extend TJSDocument and add a closing getter that always returns the correct value. In this case you want to return true. This is new in v11:

Application {
  /**
   * Whether the Application is currently closing.
   * @type {boolean}
   */
  get closing() {
    return this._state === Application.RENDER_STATES.CLOSING;
  }
}

Application {
  /**
   * Whether the Application is currently closing.
   * @type {boolean}
   */
  get closing() {
    return this._state === Application.RENDER_STATES.CLOSING;
  }
}

Also the particular app that is associated with token documents seems to be TokenConfig. --------- Just gotta say that is gnarly tech debt level coding in core in general. 😮 If things were on totally friendly terms w/ the core team I'd really like to suggest some ways of fixing event callbacks for the document model. I don't think much is going to come of that, but I'm trying soon to reach out an olive branch of sorts.

Nekro DarkmoonOP•2y ago

This is in the _onUpdate hook for tokenDocument

TyphonJS (Michael)•2y ago

Of course this new path in v11 is only being hit particularly for TokenDocument.

Nekro DarkmoonOP•2y ago

Yeah I've not come across this with most other documents

TyphonJS (Michael)•2y ago

IE what is happening is that core is hard associating API that is only in TokenConfig directly to all apps that register w/ TokenDocument. This failure or something else would occur if a different app type than TokenConfig is registered w/ TokenDocument. Super fragile and not great IMHO. Let me know if doing something like this "fixes" things / more or less it is a hack workaround.

class MyTokenDocument extends TJSDocument {
   get closing() { return false; }

   // Possibly need this returning associated token document.
   get preview() { return this.get(); }

   // This can't hurt...
   _previewChanges() {}
}

class MyTokenDocument extends TJSDocument {
   get closing() { return false; }

   // Possibly need this returning associated token document.
   get preview() { return this.get(); }

   // This can't hurt...
   _previewChanges() {}
}

Nekro DarkmoonOP•2y ago

Gonna get dinner and then I'll try it out and report back

TyphonJS (Michael)•2y ago

I am going to scan through the document model and see if this is the only instance of such unsavory hard coding to particular app types. I may not add a default closing getter to TJSDocument if this is only occuring with say TokenDocument. The fix though is just creating an override like the above specifically for use w/ TokenDocument. Then information should be added to the API documentation describing the scenario.

Nekro DarkmoonOP•2y ago

The only other thing that comes to mind that might employ this is an adventure document

TyphonJS (Michael)•2y ago

You may need to do something slightly different besides adding closing getter like faking the other API specific to TokenConfig IE _previewChanges also it looks like the preview property is the TokenDocument. I also noticed that AmbientLightConfig app has something similar.

Nekro DarkmoonOP•2y ago

Hmm I'll take a look and see what needs to be changed

TyphonJS (Michael)•2y ago

I added a few more things to example code above. I don't know what precisely is necessary, so you'll have to examine the API called on apps from TokenDocument.

Nekro DarkmoonOP•2y ago

Hmm so in the component it's able to get closing but when foundry does app.closing it comes up as undefined

TyphonJS (Michael)•2y ago

I assume you used the basic example code from above that defined the preview getter?

Nekro DarkmoonOP•2y ago

yup

import { TJSDocument } from "@typhonjs-fvtt/runtime/svelte/store"

export default class TJSTokenDocument extends TJSDocument {

    get closing() { return false; }

    // Possibly need this returning associated token document.
    get preview() { return this.get(); }

    // This can't hurt...
    _previewChanges() {}
}

import { TJSDocument } from "@typhonjs-fvtt/runtime/svelte/store"

export default class TJSTokenDocument extends TJSDocument {

    get closing() { return false; }

    // Possibly need this returning associated token document.
    get preview() { return this.get(); }

    // This can't hurt...
    _previewChanges() {}
}

console.log(target.closing);

console.log(target.closing);

Doing this in the component gives the correct value of false

TyphonJS (Michael)•2y ago

And you have verified that this.get() isn't an instance. You can also put some log statements in TJSDocument and / or provide a delete callback in the TJSDocument options to log something. If the app is "closed" TJSDocument disassociates the tracked document and removes tracking from the doc.apps object. I just posted pseudo-code on likely things required from a quick examination of TokenDocument. IE visually looking at the code and not running anything on my side. Looks like things are getting closer though. Oh... And you might need to set get closing() { return true; } or test that out. That way the if conditionals in TokenDocument hopefully don't trigger.

Nekro DarkmoonOP•2y ago

yeah so it seems to not even registering that the get properties are there 🤔

TyphonJS (Michael)•2y ago

You won't see accessors I believe as it's defined on the prototype and not an own property. This might work: console.dir(myInstance, { showAccessor: true }); Actually that is for Node...

Nekro DarkmoonOP•2y ago

okay so I can see the closing and stuff under BoundThis

TyphonJS (Michael)•2y ago

Try setting closing to true and verify that it'll skip all of the if conditionals in TokenDocument.

Nekro DarkmoonOP•2y ago

nope This is what happens when I try to get closing from apps in the console

TyphonJS (Michael)•2y ago

Well... It won't be the same UUID every time.

Nekro DarkmoonOP•2y ago

TyphonJS (Michael)•2y ago

Do an instanceof check against TJSTokenDocument. Granted perhaps not in the console. I think just adding console.log statements directly into foundry.js for TokenDocument._onUpdate and perhaps inside of TJSDocument in the Node / package source will give more info. The only place I see the additional access for a specific app / IE TokenConfig is in TokenDocument._onUpdate.

Nekro DarkmoonOP•2y ago

let t = canvas.scene.tokens.get($target.id);
let app = Object.values(t.apps)[0];
console.log(app);
console.log(app instanceof TJSTokenDocument);

let t = canvas.scene.tokens.get($target.id);
let app = Object.values(t.apps)[0];
console.log(app);
console.log(app instanceof TJSTokenDocument);

Nekro DarkmoonOP•2y ago

hmm

Nekro DarkmoonOP•2y ago

TyphonJS (Michael)•2y ago

Ooops... My bad... TJSDocument sets a bespoke object like this in doc.apps in TJSDocument.set:

         document.apps[this.#uuidv4] = {
            close: this.#deleted.bind(this),
            render: this.#updateSubscribers.bind(this)
         };

         document.apps[this.#uuidv4] = {
            close: this.#deleted.bind(this),
            render: this.#updateSubscribers.bind(this)
         };

Nekro DarkmoonOP•2y ago

will try that yeah I just found that as you posted tht 🤣

TyphonJS (Michael)•2y ago

So that is going to be annoying... What you can do is this in TJSTokenDocument:

class TJSTokenDocument {
   set(document, options) {
      super.set(document, options);

      if (document) {
          document.apps[this.uuidv4].closing = true;
          // or whatever is necessary. 
      } 
   }
}

class TJSTokenDocument {
   set(document, options) {
      super.set(document, options);

      if (document) {
          document.apps[this.uuidv4].closing = true;
          // or whatever is necessary. 
      } 
   }
}

I'm pretty hesitant to add specific code into TJSDocument to handle this case as I only see Foundry / core doing weirder thiings in the future. IE expecting all apps in TokenDocument to be a specific type of instance and then not doing any error checking before invoking things is bad form. IE if they just added some conditional chaining that would be fine.

    Object.values(this.apps).forEach(app => {
      if ( !app.closing ) app?.preview?.updateSource?.(data, options);
    });

    Object.values(this.apps).forEach(app => {
      if ( !app.closing ) app?.preview?.updateSource?.(data, options);
    });

It's possible that this can be raised on the core forums about adding conditional chaining in cases where a specific app type is expected. They can easily add that in the next v11 patch. I don't think I want to fall on this sword like the previous request to make a core change regarding synthetic actors and adding a renderContext / action. The folks on the main forum were curious as to why as core didn't break.
Though it's generally not great coding on core's part again to hard associate an app type to what is expected in any given document. You might just add in the conditional chaining for all of the app access in TokenDocument._update I bet that will fix things without any changes / workaround on the TJSDocument side of things.

Nekro DarkmoonOP•2y ago

xD I'm tempted to just set closing to be true always 🤣 But for anyone looking at this in the future

export default class TJSTokenDocument extends TJSDocument {
  set(document, options = {}) {
    super.set(document, options);
    
    if (document instanceof globalThis.foundry.abstract.Document) {
      document.apps[this.uuidv4].closing = false;
      document.apps[this.uuidv4].preview = this.get();
      document.apps[this.uuidv4]._previewChanges = () => {};
    }
  }
}

export default class TJSTokenDocument extends TJSDocument {
  set(document, options = {}) {
    super.set(document, options);
    
    if (document instanceof globalThis.foundry.abstract.Document) {
      document.apps[this.uuidv4].closing = false;
      document.apps[this.uuidv4].preview = this.get();
      document.apps[this.uuidv4]._previewChanges = () => {};
    }
  }
}

This will do the trick

TyphonJS (Michael)•2y ago

Yeah.. But that is a workaround and requires specific handling on your side. Foundry core should apply basic defensive measures as that is error prone in general / source of bugs. No where is it documented that you can only associate TokenConfig / no other apps to a TokenDocument, etc. While at it you can mention AmbientLightDocument too.

Nekro DarkmoonOP•2y ago

All 3 of them need to be set 100% might want to pin this 😄

TyphonJS (Michael)•2y ago

You might just check and verify the conditional chaining changes. I think it is a very reasonable request to make just that it's probably best that I'm not involved in the conversation. IE you can say you are a system dev that is associating an app to TokenDocument and explain the hard coded requirements added in v11 and that conditional chaining solves the problem with no downsides. Point out that for any other document types that depend on a specific app implementation that conditional chaining should be used.

Nekro DarkmoonOP•2y ago

👌 will do that in the next few days I'll make a github issue for it directly instead of posting on the discord

TyphonJS (Michael)•2y ago

Might as well make an issue, but also bring it up #v11-feedback as this is a quick / simple fix that makes their code more resilient. Do mention AmbientLightDocument as well as also having this problem / hard association to app type. Heh heh.. Also do test on your side that conditional chaining does the trick and just use a normal TJSDocument instance, etc. I do want to follow up with this as it's fairly important to raise this issue as soon as possible and have some discussion w/ the core team. @nekrodarkmoon when you have the chance do make temporary mods to foundry.js for TokenDocument._onUpdate adding conditional chaining for all embedded apps access and test your system again using a non-modified TJSDocument instance. If that works then we should proceed to inform the core team. This fix is easy for core to add and should not cause any issues thus it can quite possibly be added in the next v11 patch vs waiting for v12 if it gets implemented at all. The general problem for TRL is that the only avenue for receiving events from the core document model is to attach to the apps property of documents / ClientDocumentMixin. There is no other mechanism for event distribution in the core document model. In v11 for TokenDocument / AmbientLightDocument code was added to those documents that only expects specific apps to be added to those documents. This hard coded association to specific app types is a general problem especially if something similar spreads to other document types. It's generally good to have the issue raised by someone other than myself, but I'm certainly willing to join any conversation to provide clarity as necessary. In general I think the core team is probably somewhat resistant to make a change even though it hardens core to be more resilient when the request regards an alternate UI framework, etc. Ping me when the GH issue goes up, but I think it's something that can be raised for discussion in #v11-feedback, etc. Heya @nekrodarkmoon. Once again I'd like to follow up. I'll post an issue tomorrow and raise things on the mothership developer forums. Don't worry about all of that as it will be easier for me to succinctly describe the issue and resolution. What will really help though is if you have the time to add the conditional chaining to TokenDocument in foundry.js and test your use case with an unmodified TJSDocument instance. This will save me ~30-60 minutes from verifying this myself. Though I probably can get that verification done quicker. It's just a context switch that I'd like to avoid as I need to get the next TRL release out this week and still lots more to do with that.

Nekro DarkmoonOP•2y ago

Sorry I've just been really busy this past week. I can test out the conditional chaining when I get home after work today.

TyphonJS (Michael)•2y ago

Awesome.. I definitely appreciate it. I'm also against some time pressure here, so anything can help. I can definitely file the issue, etc. It just helps knowing that the proposed solution exactly solves your real world use case which is better than me setting up a less involved debug test.

Nekro DarkmoonOP•2y ago

👍 I'll be free in about 6 hours so I can check it then

/** @inheritdoc */
  _onUpdate(data, options, userId) {
    Object.values(this.apps).forEach(app => {
      if ( !app.closing ) app.preview.updateSource(data, options);
    });

    // If the Actor association has changed, expire the cached Token actor
    if ( ("actorId" in data) || ("actorLink" in data) ) {
      if ( this.actor ) Object.values(this.actor.apps).forEach(app => app.close({submit: false}));
      this.delta._createSyntheticActor();
    }

    // Post-update the Token itself
    super._onUpdate(data, options, userId);
    Object.values(this.apps).forEach(app => {
      if ( !app.closing ) app._previewChanges(data);
    });
  }

/** @inheritdoc */
  _onUpdate(data, options, userId) {
    Object.values(this.apps).forEach(app => {
      if ( !app.closing ) app.preview.updateSource(data, options);
    });

    // If the Actor association has changed, expire the cached Token actor
    if ( ("actorId" in data) || ("actorLink" in data) ) {
      if ( this.actor ) Object.values(this.actor.apps).forEach(app => app.close({submit: false}));
      this.delta._createSyntheticActor();
    }

    // Post-update the Token itself
    super._onUpdate(data, options, userId);
    Object.values(this.apps).forEach(app => {
      if ( !app.closing ) app._previewChanges(data);
    });
  }

if ( !app?.closing ) app?.preview?.updateSource(data, options);

if ( !app?.closing ) app?.preview?.updateSource(data, options);

if ( !app?.closing ) app?._previewChanges(data);

if ( !app?.closing ) app?._previewChanges(data);

need conditionals in 2 places with these 2 it's all fixed 👌

TyphonJS (Michael)•2y ago

NIce.. Thanks for confirming.. good to hear it is a simple fix and IMHO a reasonable one just for safeties sake since the "apps" call back registration is not well documented in general and certainly not safe if more of this kind of thing sneaks into other document types. I know this is a bit of a pain, but can you retest with the latest v11.304 They further modified _onUpdate to:

  _onUpdate(data, options, userId) {
    if ( Object.values(this.apps).some(app => app.preview) ) options.animate = false;

    // If the Actor association has changed, expire the cached Token actor
    if ( ("actorId" in data) || ("actorLink" in data) ) {
      if ( this.actor ) Object.values(this.actor.apps).forEach(app => app.close({submit: false}));
      this.delta._createSyntheticActor({ reinitializeCollections: true });
    }

    // Post-update the Token itself
    super._onUpdate(data, options, userId);
    Object.values(this.apps).forEach(app => app._previewChanges(data));
  }

  _onUpdate(data, options, userId) {
    if ( Object.values(this.apps).some(app => app.preview) ) options.animate = false;

    // If the Actor association has changed, expire the cached Token actor
    if ( ("actorId" in data) || ("actorLink" in data) ) {
      if ( this.actor ) Object.values(this.actor.apps).forEach(app => app.close({submit: false}));
      this.delta._createSyntheticActor({ reinitializeCollections: true });
    }

    // Post-update the Token itself
    super._onUpdate(data, options, userId);
    Object.values(this.apps).forEach(app => app._previewChanges(data));
  }

In this case the last line likely just needs one optional chain: Object.values(this.apps).forEach(app => app?._previewChanges(data)); --------- The same pattern is used in the updated AmbientLightDocument, so just one optional chaining for _previewChanges is probably necessary. This certainly makes it easier to justify them adding a single character ? to provide a safe fix. If anything though they are willing to modify this section of code between point releases. Err, not exactly what I'd call "stable" with such mods. It does help to have your real world test case. Ah, heck I'll just tag you @nekrodarkmoon... Thanks again!

Nekro DarkmoonOP•2y ago

XD you can tag me as many time as you want. I'm on too many servers to keep track so pinging really helps Yup it's what I tested with this the extra ?. In the second block Oh wait That seems like a different onUpdate

TyphonJS (Michael)•2y ago

I know it's the same essentially, but the code changed to where it likely just needs one optional chain usage. Yeah... v11.303 to v11.304 had the change above.

Nekro DarkmoonOP•2y ago

I'll test it when I get home in a bit

TyphonJS (Michael)•2y ago

Most excellent.. It'll be much easier to just say.. "Hey guys can you add a ? here... Thanks... " 😄 IMHO this is a worrying trend though of hard coding specific app requirements within the ClientDocumentMixin / document model constraints. v11 added this, so what document type will get the next hard coupling. Definitely an "anti-pattern" / hard coupling and makes further improvements to the document model infrastructure not as easy if this proliferates. I'd really like to propose that they get rid of this mechanism for the App v2 API and support a proper event system for the document model. Something super minimal like the Svelte / store contract w/ a single subscribe method returning an unsubscribe function is ideal, but I'll take anything that moves beyond this hard association of apps. This was around since day 1 and a hack for the initial GUI / App v1 implementation.

Nekro DarkmoonOP•2y ago

/** @inheritdoc */
  _onUpdate(data, options, userId) {
    if ( Object.values(this.apps).some(app => app.preview) ) options.animate = false;

    // If the Actor association has changed, expire the cached Token actor
    if ( ("actorId" in data) || ("actorLink" in data) ) {
      if ( this.actor ) Object.values(this.actor.apps).forEach(app => app.close({submit: false}));
      this.delta._createSyntheticActor({ reinitializeCollections: true });
    }

    // Post-update the Token itself
    super._onUpdate(data, options, userId);
    Object.values(this.apps).forEach(app => app._previewChanges(data));
  }

/** @inheritdoc */
  _onUpdate(data, options, userId) {
    if ( Object.values(this.apps).some(app => app.preview) ) options.animate = false;

    // If the Actor association has changed, expire the cached Token actor
    if ( ("actorId" in data) || ("actorLink" in data) ) {
      if ( this.actor ) Object.values(this.actor.apps).forEach(app => app.close({submit: false}));
      this.delta._createSyntheticActor({ reinitializeCollections: true });
    }

    // Post-update the Token itself
    super._onUpdate(data, options, userId);
    Object.values(this.apps).forEach(app => app._previewChanges(data));
  }

Changing to this works but still throws an error about _previewChanges not being a function.

Object.values(this.apps).forEach(app => app?._previewChanges(data));

Object.values(this.apps).forEach(app => app?._previewChanges(data));

Doing this however kills the error

Object.values(this.apps).forEach(app => app?._previewChanges?.(data));

Object.values(this.apps).forEach(app => app?._previewChanges?.(data));

@mleahy

Nekro DarkmoonOP•2y ago

TyphonJS (Michael)•2y ago

It's certainly good to add the optional chaining to the full extext. But good to know the first line if ( Object.values(this.apps).some(app => app.preview) ) options.animate = false; doesn't need it. Fingers crossed that the core team thinks it is wise to add this and not a problem. We'll see.

Nekro DarkmoonOP•2y ago

yeah

TyphonJS (Michael)•2y ago

I just have to be very careful to not suggest that this change is because of TRL / my framework, so have to word things carefully. Though it's perfectly reasonable technically to add this as any app could be added vs hard coded dependence, etc.

Nekro DarkmoonOP•2y ago

👌

TyphonJS (Michael)•2y ago

Looks like it was patched for the next release: https://github.com/foundryvtt/foundryvtt/issues/9735.

Nekro DarkmoonOP•2y ago

Very nice!

TyphonJS (Michael)•2y ago

Yeah.. Given the stability patches there definitely was some urgency in posting it and it worked out this time. Though if you saw the conversation on the mothership I do find it tiring that I get challenged when bringing up discussion. I don't often comment much and when I do it's for actual problems in general. Things went live with the v11.305 update that just came out. Give it a spin and let me know if all is good. 😄

Gaming

Programming

TJSDocument and Token Issues