Chore: rip out v9 compatibility and other chore commits #262

anthonyronda · 2022-10-26T22:04:21Z

Tests: 395/395 passing

The input delay on tests is now 120ms due to some dev machines not completing tests under the 100ms delay
V9 compatibility code has been removed due to abandoning support for V9 in releases moving forward
Some remaining references to the data object on documents have been removed
Removed some errant console logging that accidentally got committed to the system
returned the dependency caching to the CI workflow
I placed the --ignore-scripts flag on the npm ci command after verifying locally that current dependencies do not have required postinstall scripts for npm run build to function properly

wyrmisis

Great set of changes. I have a bunch of conceptual questions and "while we're in there" fixes to suggest.

src/module/actor/actor-sheet.js

wyrmisis · 2022-10-26T22:11:58Z

src/module/actor/actor-sheet.js

@@ -122,7 +122,7 @@ export class OseActorSheet extends ActorSheet {
   */
  _useConsumable(event, decrement) {
    const item = this._getItemFromActor(event);
-    const itemData = item?.system || item?.data?.data; //v9-compatibility
+    const itemData = item?.system;

    if (decrement) {
      item.update({ "data.quantity.value": itemData.quantity.value - 1 });


We should replace these data.[...] updates with system.[...].

Yes, thanks for the catch. If I haven't caught every remaining data reference, I've at least taken care of the ones in document update methods in today's first commit

wyrmisis · 2022-10-26T22:16:17Z

src/module/actor/actor-sheet.js

@@ -187,7 +187,7 @@ export class OseActorSheet extends ActorSheet {
    let element = event.currentTarget;
    let attack = element.parentElement.parentElement.dataset.attack;
    const rollData = {
-      actor: isNewerVersion(game.version, "10.264") ? this : this.data, //v9-compatibility
+      actor: this,


Isn't this the OseActorSheet (or subclass) here?

this is OseActorSheetCharacter. _rollAttack() is only used for "improvised melee" and "improvised ranged" (by clicking on the MEL and MIS headers of the melee and missile bonus boxes).

Because it appeared targetAttack() will take either one for the roll data without complaint, I looked into the reason...it's completely ignored

removed rollData as irrelevant in my third commit today

wyrmisis · 2022-10-26T22:17:23Z

src/module/actor/actor-sheet.js

@@ -187,7 +187,7 @@ export class OseActorSheet extends ActorSheet {
    let element = event.currentTarget;
    let attack = element.parentElement.parentElement.dataset.attack;
    const rollData = {
-      actor: isNewerVersion(game.version, "10.264") ? this : this.data, //v9-compatibility
+      actor: this,
      roll: {},
    };
    actorObject.targetAttack(rollData, attack, {


Thoughts on setting rolldata on the actor instead of the actor sheet? Do we need to give rollData the actor sheet?

rollData has been removed from the function (see last comment)

wyrmisis · 2022-10-26T22:18:10Z

src/module/actor/actor-sheet.js

@@ -204,7 +204,7 @@ export class OseActorSheet extends ActorSheet {
    const dropTarget = event.target.closest("[data-item-id]");
    const targetId = dropTarget ? dropTarget.dataset.itemId : null;
    const target = siblings.find((s) => s.data._id === targetId);
-    const targetData = target?.system || target?.data?.data; //v9-compatibility
+    const targetData = target?.system;


Should we bail if there's no target?

I think there will be a merge conflict with your item data model code here
For now, I did decide to throw an error which might be a good idea in other places where we can think of a useful error to write for debugging

wyrmisis · 2022-10-26T23:04:06Z

src/module/item/item-sheet.js

@@ -72,11 +72,11 @@ export class OseItemSheet extends ItemSheet {
      this.object.popManualTag(value);
    });
    html.find("a.melee-toggle").click(() => {
-      this.object.update({ data: { melee: !this.object.data.data.melee } });
+      this.object.update({ data: { melee: !this.object.system.melee } });


this.object.update({ 'system.melee': !this.object.system.melee })

wyrmisis · 2022-10-26T23:04:48Z

src/module/macros.js

@@ -15,7 +15,7 @@ export async function createOseMacro(data, slot) {
    return ui.notifications.warn(
      game.i18.localize("OSE.warn.macrosOnlyForOwnedItems")
    );
-  const item = data.data;
+  const item = data;


Why not just rename the param above to item?

oops, turns out this is not an Item document object after all! So this code change breaks the (probably not often used) ability to drag an item from your sheet to your macro bar to create a roll macro (or if there is no roll for the item, a "view item" macro)

This change has been undone. I'd like to refactor this as part of a different PR

wyrmisis · 2022-10-26T23:05:43Z

src/module/party/party.js

-          act.data.flags[systemName] &&
-          act.data.flags[systemName].party === true);
-    console.log(characters)
+    const systemName = game.system.id == "ose" ? game.system.id : "ose-dev";


Can this come from CONFIG.OSE?

addressed in third commit today named "fix: one-use keys..."

wyrmisis · 2022-10-26T23:06:32Z

src/module/party/party.js

+      (act) =>
+        act.type === "character" &&
+        act.flags[systemName] &&
+        act.flags[systemName].party === true


Could this prop intentionally be a truthy value that isn't true

I did a cursory check of setFlag in the repo and only found "party" only ever getting set to true or false

wyrmisis · 2022-10-26T23:09:35Z

src/templates/apps/party-sheet.html

      {{localize 'OSE.dialog.xp.label'}}
    </button>
    {{/if}}
  </div>
  <div class="body party-members">
-    {{#if user.isGM}}
-    {{#if (eq partyActors.length 0)}}
+    {{#if user.isGM}} {{#if (eq partyActors.length 0)}}


Rather than nesting ifs, could we add a prop to the backing JS file for this?

Hope you'll accept this solution to eliminate nesting ifs
The and helper is from foundry.js

anthonyronda · 2022-11-29T15:19:33Z

still working on this PR

There's a problem with drag and drop I'm starting to diagnose and I don't know where they got introduced

wyrmisis · 2022-12-06T01:37:00Z

src/module/actor/actor-sheet.js

+    itemIds.push(itemData.id);
+    const item = this.actor.items.get(itemData[0]._id);
+    await item.update({ system: { containerId: container.id } });
+    await container.update({ system: { itemIds: itemIds } });


There are some places where we return promises, and others where we await promises in place at the end of a function. I don't think we have to standardize it for this PR, especially if we're not sure how long this sheet is going to stick around, but it might be good to hammer out the pattern we want to use.

wyrmisis · 2022-12-06T01:42:31Z

src/module/actor/actor-sheet.js

@@ -456,11 +402,11 @@ export class OseActorSheet extends ActorSheet {

    if (event.target.dataset.field === "value") {


We could probably DRY up blocks like this so we only call item.update in one spot, like so:

() => { const {field} = event.target.dataset; if (field !== 'value' && field !== 'max') throw new Error('Malformed update call when updating item quantity'); return item.update({ [`system.quantity.${field}]: parseInt(event.target.value) }); }

Not worth holding the PR up for, just a thought

wyrmisis

Looks good to me

anthonyronda added 4 commits October 26, 2022 16:18

build: disable postinstall scripts, enable cache

91199b5

chore: bump delay, remove unused var

48856e2

chore: remove v9 compatibility code and data objs

b7e6847

chore: removed test logging

fa6027b

anthonyronda requested review from wyrmisis and maschill92 October 26, 2022 22:04

wyrmisis requested changes Oct 26, 2022

View reviewed changes

anthonyronda added 6 commits October 28, 2022 17:09

fix: more references to document data obj removed

3beda96

fix: lessened verbosity of _useConsumable()

c8c6b3b

fix: removed unused rollData in _rollAttack()

ae92785

fix: item rolls not working on monsters

52afea8

fix: code style cleanup

3e8b40e

fix: one-use keys no-longer defined in consts

e6f1b04

anthonyronda mentioned this pull request Nov 23, 2022

Enforce linting #266

Closed

anthonyronda added 6 commits November 22, 2022 22:40

fix: removed unnecessary optional chaining on this

5b9c0f8

fix: implement wymisis's _resetAttacks code golf

ee380d6

fix: removed more unnecessary optional chaining

1146ef4

fix: removed data reference to object update method

7dd3da5

fix: this data.data is not a FVTT V9 API document

6d88d58

fix: throw an error if target can't be found

1289168

fix: removed erroneous .id prop from string filter

610c5af

anthonyronda mentioned this pull request Dec 1, 2022

Deprecation warnings in V10 #211

Closed

5 tasks

anthonyronda added 5 commits November 30, 2022 19:13

chore: code style, don't push to a const array

6394ea7

fix: base aac should be 10 for no-target attacks

ff8e483

fix: use uuid for treasure table link

1cb83a9

chore: code style, no nesting ifs

f4916a1

code style: removed extraneous use of key:val

f34c0cd

anthonyronda requested a review from wyrmisis December 1, 2022 01:45

wyrmisis reviewed Dec 6, 2022

View reviewed changes

wyrmisis approved these changes Dec 6, 2022

View reviewed changes

Merge branch 'main' into chore-rip-out-v9-compatibility

ed6477d

anthonyronda merged commit f7dbbdd into vttred:main Dec 10, 2022

anthonyronda deleted the chore-rip-out-v9-compatibility branch December 10, 2022 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: rip out v9 compatibility and other chore commits #262

Chore: rip out v9 compatibility and other chore commits #262

anthonyronda commented Oct 26, 2022

wyrmisis left a comment

wyrmisis Oct 26, 2022

anthonyronda Oct 28, 2022

wyrmisis Oct 26, 2022

anthonyronda Oct 28, 2022

wyrmisis Oct 26, 2022

anthonyronda Oct 28, 2022

wyrmisis Oct 26, 2022

anthonyronda Nov 30, 2022 •

edited

Loading

wyrmisis Oct 26, 2022

wyrmisis Oct 26, 2022

anthonyronda Nov 23, 2022

anthonyronda Nov 29, 2022

wyrmisis Oct 26, 2022

anthonyronda Nov 23, 2022

wyrmisis Oct 26, 2022

anthonyronda Dec 1, 2022

wyrmisis Oct 26, 2022

anthonyronda Dec 1, 2022

anthonyronda commented Nov 29, 2022

wyrmisis Dec 6, 2022

wyrmisis Dec 6, 2022

wyrmisis left a comment

		@@ -456,11 +402,11 @@ export class OseActorSheet extends ActorSheet {

		if (event.target.dataset.field === "value") {

Chore: rip out v9 compatibility and other chore commits #262

Chore: rip out v9 compatibility and other chore commits #262

Conversation

anthonyronda commented Oct 26, 2022

wyrmisis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonyronda Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonyronda commented Nov 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyrmisis left a comment

Choose a reason for hiding this comment

anthonyronda Nov 30, 2022 •

edited

Loading