Rework dnd5e class imports to avoid using non-relative paths

[Ben Freeland]

In a number of places classes are imported from the dnd5e system using a relative path.

Example from item.js:

import Item5e from '../../../../systems/dnd5e/module/item/entity.js';

Example from npc.js:

import ActorSheet5eNPC from '../../../../systems/dnd5e/module/actor/sheets/npc.js';

This sort of reference is a bit of a violation of best practices, as some hosting services such as the Forge implement a sort of sandboxing of modules which normally prevents these sort of relative path references. This doesn’t cause a user affecting bug currently because the The Forge added a manual exception for the Obsidian module.

Instead of asking for an exception for my Obsidian fork I was hoping to rework these imports to remove the relative references and then provide a patch that could bring in those improvements to the mainline. Unfortunately I’ve hit a bit of a wall and was hoping you might have some ideas for how to accomplish this.

Using some tips from other devs I was able to remove some of the imports such as Item5e by using the global game.dnd5e references such as game.dnd5e.entities.Item5e. This method doesn’t work for imports used as the parent of an extending class, because global references such as game.genefunk.applications.ActorSheet5eNPC aren’t valid in that context.

[Kim Mantas]

Must be pretty normal to import classes from the system that your module depends on. How do other modules do it?

[Ben Freeland]

I went through a bunch of sheets modules and while most of them used the relative path import, I did find one that seems like it might have a good alternative.

Loot Sheet NPC PF2e (https://gitlab.com/bipedalshark/loot-sheet-npc-pf2e)

In their lootsheetnpcpf2e.js script they extend the ActorSheetPF2e class like this:

const ActorSheetPF2e = Object.getPrototypeOf(
    CONFIG.Actor.sheetClasses.loot["pf2e.ActorSheetPF2eLoot"].cls
  );
  class LootSheetNPC extends ActorSheetPF2e {

I’ll do some tests and see if I can reverse engineer what’s going on and do something analogous.

[Ben Freeland]

So the full context of the lootsheetnpcpf2e.js is

Hooks.on("ready", async () => {
  const ActorSheetPF2e = Object.getPrototypeOf(
    CONFIG.Actor.sheetClasses.loot["pf2e.ActorSheetPF2eLoot"].cls
  );
  class LootSheetNPC extends ActorSheetPF2e {
    ...
  }


  //Register the loot sheet
  Actors.registerSheet("pf2e", LootSheetNPC, {
    types: ["npc"],
    label: "LootSheetNPC",
    makeDefault: false
  });
};

So it seems the reason this works is because the sheet isn’t defined until after the pf2e sheets have been registeredsheetClasses have been registered. At that point their classes can be fetched anywhere. The downside of this is that you can’t export a sheet if you delay it’s definition like this, which means you aren’t able to import it even though it’s part of your own module. Sooo not ideal.

‌

[Kim Mantas]

Yes, why even bother using ES6 modules at that point? There’s no way this can be considered ‘best practice’.

With some experimentation I was able to wrangle my IDE to change system imports to look like this: /systems/dnd5e/module/actor/sheets/npc.js which seemed to work correctly. I also tried a variation without the leading / but that failed to load in foundry.

I have no idea if these absolute paths will work as expected in every case though. I can imagine if foundry is hosted on some subdirectory URL, it might break.

What we really need is an example of a ‘correct’ import path to use that will work for standard foundry, for self-hosted instances, and across the various hosting platforms. However, I think routing rules can be entirely arbitrary, so such a thing might not even exist.

You mentioned that most other modules you looked at seem to use these relative paths. Do they all require exemptions too?

[Ben Freeland]

Apparently the module developers only started recommending this new method of system imports a couple months ago and the other modules that still do it the old way either don’t work with Forge (like me wasn’t) or they’ve been given an exception like Obsidian.

I was able to solve this on my fork and successfully removed all sandbox breaking imports! Now it’s working perfectly with The Forge’s Bazaar.

The trick to solving the problem with extending base classes was to delay import of those classes within the module’s .js files to inline instead of at the top of the file: https://bitbucket.org/finalfrog/obsidiangenefunk/commits/76076bef377b6f49fb4748c983dcd98f675ef163

I’ll write up a patch for equivalent changes to the base Obsidian sheet that you can choose to pull in if you like, but it won’t work until the dnd5e maintainers add a patch to expose the ActorSheet5e class via game,dnd5e.applications.ActorSheet5e. I had to fix that in my dnd5e fork: https://gitlab.com/finalfrog/genefunk/-/commit/fc91da2771fe5cf52db1480a214fa8e90defbcec

[Ben Freeland]

Update: Every alternate way of importing from the system I’ve tried has resulted in an error, often one that didn’t manifest until using a particular feature on the sheet. In the interests of maintaining stability I’m gonna call it quits for the time being until a better solution comes up or one of my sheet’s users wants to use Forge. You can probably close this ticket.

[Kim Mantas]

Alright, well, thank you for looking into this and keeping me updated. You’re probably more familiar with the issue by now but my intuition is that this sandboxing problem would be better solved by some routing rules on the hosts' side, rather than requiring module developers to chop up their codebase and load everything dynamically at run time. It’s not very ergonomic and means you lose out on a lot of IDE/Intellisense support.

How difficult is it to get an exemption on the Forge, by the way?

[Ben Freeland]

Not too hard. The dev offered to give me an exception when I described the issues I was having with early attempts, I said I’d rather get it working the “right” way if possible. Probably going to take them up on it now.