monkeypatching vs extending

[shy cipher]

Monkeypatching is preferable for modules, since they can't extend classes without breaking systems that also extend them, unless they have specific support for that system or do something weird.

[spring geyser]

as explained before, systems usually register their classes during the init hook (or even on load time), so imo, it's fine for modules to extend classes at that time. That was also the point of the above api.

[lucid crypt]

I've written a lot of modules, and I've never once monkeypatched, so I'm not sure if I've somehow just managed to only ever have ideas that don't need it, or if I'm somehow doing something different

the core way of registering multiple classes is sheetClasses unless I'm misunderstanding your example, although I'm not sure if Token actually uses said feature, their parent Actor does

[shy cipher]

I have need for it in bunch of modules, and other bunch of them don't need it.
Only one that does it with Foundry itself tho (IIRC).
Extending might be doable (via CONFIG.Canvas.layers.grid.layerClass ?), I suppose even in that case.

Problems come when you need to patch a class you have access only after it has been used already, or people use new ClassName path for.

Other problem with extending the classes with modules is that it masks the system class name, if the system already extends it, though that's not much of a problem except for debugging.

[spring geyser]

I wasn't talking about registering multiple sheets, I was talking about multiple different modules making changes to the same document (or object, or layer, or whatever...) class

One situation where i literally cannot get around it in FXMaster at the moment is that I have to patch some Canvas methods (https://github.com/ghost-fvtt/fxmaster/blob/master/src/wrappers/canvas.js#L6). Other situations are where I want to avoid the complicated dynamic extending I explained above, while also trying to stay compatible with other modules /systems (e.g. https://github.com/ghost-fvtt/foreground-drawings/blob/master/src/wrappers/drawing-document.ts#L10, but there are a lot of examples)

another situation where I don't see a way to do it without monkey patching is my keyboard-layout module. I need to make some changes to ClientKeybindings, but from my understanding, there is no way to configure a custom class to be used for that. (https://github.com/ghost-fvtt/keyboard-layout/blob/master/src/wrappers/clientKeybindings.js#L10)

There are many such situations in a lot of modules. Granted, a lot of them are similar to my second example, so they could potentially be replaced by extending classes, but it's also just less ergonomic. And if you are using libWrapper instead patching by hand, there isn't really a big downside, imo

[lucid crypt]

if you're going to monkeypatch, definitely do use Libwrapper, I agree - it means there's at least some user control

but for most of these cases, we would prefer that you reach out with requests to expand our APIs to add more extension points so we can reduce patching over time

[spring geyser]

so "please allow us to configure a custom Canvas class" would be a valid request?

[lucid crypt]

yep!

[spring geyser]

the problem is: I don't think there is any class that we wouldn't want to be configurable 😄

[lucid crypt]

between things like registering sheets and Hooks, most of Foundry is pretty extendable already

new code, such as Tours, is being written with extendibility in mind

[late sparrow]

could I ask for an example so I know the intended way to go about things?
Currently in Border Control I overwride Token.prototype._refreshBorder which is soft marked as private. How would I go about doing this in the optimum way ?

[spring geyser]

I agree that a lot of parts that are commonly changed by modules are very extendable. But There are so many modules that probably there isn't a single class that isn't changed by some module. and there are still lot's of classes that aren't configurable

[lucid crypt]

we can definitely improve, especially in older code

[spring geyser]

if you want to avoid the monkey patch while staying compatible with other modules and the system, you could do

Hooks.on("init", () => {
  class MyToken extends CONFIG.Token.objectClass {
    /** @override */
    _refreshBorder() {
      /* your implementation, which would call `super._refreshBorder()` if possible, to be as compatible as possible */
    }
  }

  CONFIG.Token.objectClass = MyToken;
});

[late sparrow]

ok cool, thanks for the lesson 🙂

[lucid crypt]

as another thought, refresh is the public method that calls _refreshBorder, so you could override that, copy most the flow, and trade out the native _refreshBorder call for your own, although replacing the smaller method is better for inter-module compatibility

I'm not sure why _refreshBorder is private while refreshHUD is public tbh

a chunk of existing code should move to being @protected in cases like this

[late sparrow]

how does _, @ and # differ?

[spring geyser]

see #dev-support message

# is not described there because it's new

# is a JavaScript feature that actually enforces not being able to access the property from outside. More discussion about that can be found in #960677395319357480

Here is a short list of classes I need to change in some of my modules, which are currently not configurable (AFAIK):

• Canvas
• DrawingHUD
• ClientKeybindings
  So consider this a request to make them configurable 😄

[lucid crypt]

Gitlab issues or it won't happen

[lucid crypt]

just added # to the list

[spring geyser]

I think from the perspective of the dev community, there is no need to change the status quo. Most are pretty happy to work with libWrapper. But I'll still try to write up a GitLab issue when I find the time.

[crimson valve]

I think that if core cares about this, there needs to be lots more effort to explain how it works without libWrapper. I think I'm still a bit confused even after reading the whole thread.

How do you get automatic conflict detection by extending classes? How do you allow the user or other developers to suppress those detected conflicts?

How do you get automatic prioritization of which changes should be prioritized? (e.g. with libWrapper it's "WRAPPER" > "MIXED" > "OVERRIDE" and the user can configure it beyond that)

How do you remove your changes to revert back to the default (or revert to another module's implementation if one exists) later on in the program?

Most importantly, how do you do all that in a readable and consistent way so that other developers understand what is going on and so that users have a unified UI for configuring it?

[spring geyser]

I don’t think there is a way to do it on a per method basis with extending

[crimson valve]

Well, that's an issue....

[spring geyser]

You could do it on a per class basis though, by having a system that keeps track of all the classes that have been registered

[crimson valve]

It's important for one module to be able to switch out one of a class' methods, while another one modifies a different method. If one of those modules break, it shouldn't break the other module.

[spring geyser]

If it’s just one method in a class, it’s not an issue. It becomes problematic when you want to change multiple things in one class, but only one method actually results in a conflict. Because then the change to the whole class becomes incompatible

Though, it’s pretty likely that things won’t work correctly anyways if conflicts are detected, so it may not actually be that big of an issue

[crimson valve]

For my own uses of libWrapper:

I used it in my SCS module, for a feature that I call "action locking". Basically, I want to be able to decide whether to continue chaining the call depending on certain conditions. This is for CONFIG.Item.documentClass.prototype.roll. As far as I understand, I would need an API for configuring the Item document class?

[spring geyser]

Personally, I have never actually run into conflicts detected by libWrapper. But I don’t run lots of modules, so I may be the exception here

[crimson valve]

Yeah. When I am running my full load of modules, there are quite a few conflicts and the UI that ruipin provides is incredibly helpful.

[spring geyser]

Currently, that API is just setting CONFIG.Item.documentClass. But we have been discussing a bit how a more flexible api could look like

[crimson valve]

I would have to modify the entire Item class just to do this? What about compatibility with all of the modules that are assuming that it's called Item5e?

Would it also be necessary to create my own Combat class for CTG so that I can implement group initiative? How would I do that and still stay system agnostic?

How would you remove modifications made by other modules?

[spring geyser]

If any module assumes a class is called a certain name, that’s a very bad thing in general.

Do you know of any module that makes this assumption?

[crimson valve]

Not offhand, no

[spring geyser]

I would expect the number to be very low. Because this is very prone to breaking anyways.

Aside from that, I don’t think it’s that much more overhead.

If you read carefully above, you will notice that i actually argue in favor of using libwrapper for these kinds of things.

But imo it’s not as bad as you describe it here

[crimson valve]

What about places where I am wrapping a method that is called very early (maybe even before init)?

libWrapper provides a hook for that

[spring geyser]

The order of execution is a problem I also mentioned already. In order for this to work, the registration probably needs to be done at old time, not during the init hook. But there isn’t really anything that prevents doing this

Apparently, pf2e registers it’s classes at load time, as stwlam anecdotally mentioned

[crimson valve]

My Persist Sheets module doesn't use libWrapper, but applies some changes to the DocumentSheets that it opens:

const debouncedSave = debounce(() => this.save(sheet), 500);
sheet.setPosition = (...args) => {
    debouncedSave();
    sheet.constructor.prototype.setPosition.apply(sheet, args);
}
sheet.close = (...args) => {
    if (!game.settings.get(PersistSheets.ID, "keepClosedSheets")) this.delete(sheet);
    sheet.constructor.prototype.close.apply(sheet, args);
}

How would I do this without monkeypatching?

[crimson valve]

Which registration are you referring to here?

[spring geyser]

setting CONFIG.Actor.documentClass etc, or if there was a more sophisticated mechanism to override classes, that mechanism. Like what we describe above. There, I called it during the init hook, but that's not necessary. You could also call it at load time, if it's set up properly for that

[spring geyser]

this is arguably pretty dangerous... What if your override of close or setPosition clashes with another module? Won't be caught by libWrapper... is it not possible to register things dynamically with libWrapper?

I feel like there should be a much better and safer solution to what you are doing here...

[crimson valve]

Maybe, but I don't know it 😅

[lucid crypt]

@robust oak

[robust oak]

ty

[gloomy geyser]

for whatever it is worth (only saw this discussion now), there is no reason a libWrapper-like API couldn't be written and/or even included in Core that handles conflicts at registration similar to libWrapper, i.e. rather than extend classes devs would register mixins, plus metadata (like WRAPPER/MIXED/OVERRIDE) and then Core would wait until some later hook and reorder said mixins as necessary to achieve the necessary compatibility needs. Add a UI for the user to reorder things, etc, and you're good.

You lose out on dynamically detected conflicts, as well as dynamic registrations/unregistrations (modules would need to e.g. register these mixins at init), and less important stuff like per-instance wrappers, but it is arguable whether those are really that important. The important libWrapper functionality for any supported class would effectively be maintained.

heck, there's no reason libWrapper or a similar API couldn't be maintained by Core themselves
(which might finally give them the chance to fix the various UX issues that are inevitable with a hobbyist project by a non-webdev like me, even if I've spent a lot of time trying to improve it, UX really isn't my strong suit; and unfortunately it seems nobody has bothered to submit UX PRs 😛 )

if Core accepts that monkey-patching is an important (yet not always desirable) part of modding Foundry, then providing multiple layers of officially-supported extension APIs definitely makes sense. You could then point people towards the easier-to-maintain ones, while still offering a "controlled" way to use the more flexible (but harder to maintain) ones.

1. Class extension (doesn't scale well to multiple modules due to conflicts etc; but very effective if used by systems)
2. Hooks (Most easy to support; most easy to avoid conflicts in; but requires manually coding every hook point so not very flexible for module writers)
3. Mixin registration (scales well to multiple modules with reordering/conflict resolution; very flexible for module writers; better performance than libWrapper; disadvantage is that it requires modules to register everything early on during load; and doesn't support e.g. dynamic conflict detection)
4. Monkeypatch registration (scales well to multiple modules with reordering/conflict resolution; most flexible for module writers; can be done dynamically at runtime; essentially libWrapper)

Right now, #1 and #2 are officially supported and recommended by Core

#3 would be a possible future feature, I guess (or a module -- like libWrapper -- could do it?); it's essentially a more flexible #1

and then #4 right now is libWrapper

With #3 I can imagine the need for libWrapper would virtually disappear - only very few cases I can think of where libWrapper would be needed over #3 , at least.
Heck, you could probably convert almost all libWrapper calls into #3 calls automatically. Very few modules need to call e.g. libWrapper.register after init.

[spring geyser]

Thanks @gloomy geyser, you pretty much summarized my thoughts on this

[crimson valve]

I could imagine needing to do this if you are trying to mod a class that belongs to a module and that is declared after init (e.g. on ready) which is something modules can be required to do in order to properly extend the system class, for example.

[gloomy geyser]

I'm assuming they could just use the "mixins system" instead, rather than extending the system class directly

assuming systems play along, of course

but yes, there are exceptions where #4 (libWrapper) is still useful, and most of them are due to "someone isn't playing along", which in time would probably reduce in significance

fwiw, I can imagine a "dynamic conflict detection mode" even with the mixins system, so the only loss of functionality vs libWrapper that would be important would be the "wrap at any point"

[crimson valve]

and being able to unregister at any point

[gloomy geyser]

yes

though almost nobody uses libWrapper.unregister

like, maybe one or two modules out there lol

I know for a fact I've broken it accidentally before for like an hour or two, and nobody noticed 😂

[crimson valve]

Apparently I use unregister_all in Custom Fonts, but that's sort of a hack which I could remove without loss of functionality

[gloomy geyser]

also, most uses of unregister could probably be replaced with a if(disabled) return wrapped() at the beginning of each wrapper - only disadvantage is the performance cost of the extra call

anyway, whether #3 or #4 is best is honestly a matter of preference, there's no real reason to prefer #3 over #4 other than that it feels less hacky (since you control the classes that are being customised). The biggest advantage here would be Core focusing on first-class support / UX for a "generic customisation API".

I guess #3 would could you get cleaner stack traces too, which could be nice

I had brainstormed in the past adding libWrapper support for something like the mixins approach, but didn't have the time/patience (yet?)

since libWrapper already hooks into the initialisation phase, and most of the mechanisms are there, it could easily set up the necessary mixin support mechanisms at the appropriate time

[crimson valve]

Is there anything about #3 that can only be done by core?

[gloomy geyser]

mostly avoid hacks

having Core declare the points manually where mixins would be mixed in, rather than having to force e.g. libWrapper to assume everything needs to be built at init

Core could also easily add hooks like <class>MixinInit or whatever at the appropriate point in time, for classes it supports

doing it in e.g. libWrapper also means modules need to be able to provide the actual base class object that will be modified, or I need to manually code in each class myself. Would be easier in Core to have Core themselves define which classes they want to support the mixin registration, and at which point they get reordered

(basically, before the first use of a given class, the class would need to be customised)

could also add error checking, to e.g. prevent a class being instantiated before the custom mixins get mixed in. Basically difficult to do that in e.g. libWrapper

tl;dr: Nothing critical, but some QoL stuff

plus of course people who know how to actually do UX could focus on the UX
(but there's no reason you'd need #3 for that; libWrapper could also use some UI cleanup, but I don't have much experience/talent when it comes to more front-endy stuff like UI/UX. I also work for a living and haven't had a ton of time to spare lately. PRs welcome I guess?)

which goes back to my point that it doesn't really matter what approach you take, but if it is accepted that monkeypatching is a necessary evil, I think the best thing Core could do would be to actually give module devs the tools to do so in a way that has first-party support, UX, etc; instead of relying on the community (me and module devs) to figure something out.

Whether that means #3 (a mixins registration approach) or #4 (a libWrapper replacement), or even both, it doesn't really matter, and is down to preference/ideology IMO.

It's just that, while I understand the causes (and don't take it personally), the current non-committal middle-ground that I sometimes hear which goes something like "we accept monkeypatching is necessary, but you're on your own, also libWrapper causes users confusion and I kind of wish people didn't use it" feels a bit inconsistent / dismissive.

If you accept it is necessary, then (in due time, not saying it has to happen right now -- libWrapper works well enough) it should have first-party support, so that you can minimise the impact to users (by e.g. handling the UX).

[crimson valve]

I don't feel like the core team has presented a sufficient or valid alternative to libWrapper at all, but that may be my misunderstanding

[gloomy geyser]

yes, and they've multiple times stated they do consider monkey-patching/libWrapper a necessary evil - the problem is, that they provide no support or official way of doing it; which indirectly causes some of the issues they then complain about (such as bad user experience; or increased support requests). Which is fair, especially for "unstable" software in heavy development, but IMO it's something that should be resolved at some point

LibWrapper was an attempt to do this by someone from the community, for the community; but it was never intended to be the end-all-be-all solution to the module interoperability problem. Just something that could bridge the gap until there was official support, that was better than the status quo at the time (everyone on their own; with module compatibility even more of a crapshot than right now).

Mainly, I was trying to solve the problem where one of my modules, less than 100 lines long, kept getting reports of incompatibilities with other modules... I got tired after like 3 incompatibility reports with corresponding compatibility hacks needing to be added... And wrote 2000+ lines of code as a solution 🤨

also, I can't be the only person somewhat worried about an important compatibility framework used by hundreds of modules being solely reliant on a single dude maintaining it in his spare time

also, just to clarify, this isn't meant to be a rant, especially not in the negative sense. I'm happy supporting libWrapper, and I don't mind too much it being such an important cornerstone of many high-usage modules. If anything, having a relatively high-profile and high-complexity github project used by many other people helps my CV 😉

It's just that looking at it from the perspective of the Foundry VTT software, I'm not sure it is a status quo that should be desirable long-term, and thought it was a good opportunity to write down some of my thoughts. But I don't work/speak for Foundry, so maybe they disagree.

[spring geyser]

Well said, couldn’t agree more!

[livid heath]

It doesn't feel right to me that we would provide a distinction between a public/private/protected API, but then provide a public and officially sponsored way to patch the private/protected components of that API.

[crimson valve]

What about a way to patch the public components of it?

Wouldn't patching protected components via mixins be permitted by the API contract for protected methods since it's in a child class?

You should only override this in a child class

As for private components which are commonly patched in modules, I think it would make sense to transition those to protected or public when possible.

[gloomy geyser]

for reference, that is exactly why I suggested the "mixins registration" API idea, because extending classes is officially supported by Foundry right now, e.g. for systems, it just doesn't scale well to multiple modules. Registering mixins and then having the mix-in order be reodered dynamically sometime during load, similar to what libWrapper does at runtime, would permit the "extend classes" method for patching Core to scale up to multiple modules.

(Right now, if a module wishes to use the "extend classes" way of patching Core, it needs to extend from the base class and immediately replace it inside CONFIG, which means that compatibility is a crapshoot if more than one module wants to do this and isn't just doing very basic things with it.)

Then you're effectively not really adding a completely different "official way" of monkeypatching, at least nothing systems etc aren't expected to do right now. You're just telling people to extend classes rather than full-on monkeypatch, while giving them the tools to do so for almost all use cases of monkeypatching right now, without major loss of compatibility.

once full-on monkeypatching is no longer necessary for 99.9% of use cases, full-on monkeypatching could then be reserved to the very corner-case "hacky" use cases, which could then be majorily discouraged and potentially even have "special review" required for said modules to be listed

the "mixin registration" idea is, to put it simply, a way to add something almost equivalent to full monkey-patching support without going very far from what systems are already officially expected to do, which already has official support. Without redesigning the entirety of Core.

I can imagine something like (based on a random example of a libWrapper wrapper from one of my modules)

Hooks.on('init'. () => {
  registerMixin("some-module", Drawing, (parent) => class SomeModuleDrawing extends parent {
    get defaultOptions() {
      const res = super.defaultOptions;
      res.height += 70;
      return res;
    });
  }, {type: CONSTS.WRAPPER});
});```
with Core then, at some point (maybe at first instantiation of `Drawing`?) preventing further registrations, and using the metadata of the registrations (in this case `{type: WRAPPER}`) to decide which order to mix them into the parent class in a way that increases the likelihood of compatibility. Can have types (e.g. `WRAPPER`, `NORMAL`, `OVERRIDE` etc), and follow same ideas as libWrapper where you can only have one `OVERRIDE` (anything else gets reported to user somehow as incompatible), etc etc

Possibly the class parameter, rather than being the class object itself, it would be a string identifier of the class, so that people can use this system without needing to have access to the class declaration. With this, you might even be able to add support for modules to offer up their classes for overriding, e.g. `registerClassForMixin("some-module", "SomeModuleClass", SomeModuleClass)` or something fancier like
```js
class SomeModuleClass extends ExtensibleClass("some-module", "SomeModuleClass") {
   /* ... */
}``` with the `ExtensibleClass` definition being provided by Core and automatically registering the class for overrides, and allowing overrides until some later point, e.g. the first instantiation.

Solves another issue, which is adding an easy way for modules to allow extensions of their own code, which right now often requires cross-module imports...

You could then build a UI system to manage these, e.g. users being told about which modules are registering which mixins, and allowing the user to reorder mixins for compatibility purposes, for example.

If you wanted to go even further, you could also have a conflict-detection mode that works similar to libWrapper, by placing mixins inbetween each of the registered mixins, that check whether modules that report e.g. just being wrappers actually chain the APIs properly... I.e. dynamic conflict detection. Essentially what libWrapper does (although it doesn't use JS's class mechanism) when you have high-performance mode disabled.

Using some tricks like what libWrapper does, it is even possible for the package ID to be auto-detected / checked, so that packages don't accidentally (or on purpose) register mixins as other packages.

And nowhere here are you actually telling users to monkey-patch, you're just giving them a scalable way of achieving essentially the same thing.

(Yes, I have thought about this kind of thing more than I care to admit. It has been in the back of my brain for almost a year now as the potential route for a possible libWrapper v2 API, I've just been busy/lazy and as such haven't actually played around with it yet. Also, Core doing it would allow some extra niceties, like blocking further mixin registrations on first instantiation)

[crimson valve]

Sounds like it would be great DX. I definitely think that it would be super cool to be able to expose portions of a module or system like this since it makes providing a sufficient API much less difficult 💯

[livid heath]

I'm happy for folks to brainstorm and circulate ideas, and I am reading through all of this - but I feel compelled to temper expectations here

I don't see us going in this direction in foreseeable (i.e. V10 or V11) future

[gloomy geyser]

oh, I stated that above (but tbf, I write a lot of things...), this isn't a critical priority, I just think some official "monkeypatching-like" support at some point is something that should be in the plans, and not just something for like 2040

I might still play around with this a bit in the future, best case I might manage to design a libWrapper-like API for mixins that would make developers changing to whatever Core delivers in the future easier (assuming Core ever does). This sounds like a fun project/learning experience, akin to what libWrapper was previously.

But I've been quite busy with a bunch of other IRL stuff, haven't really felt in the mood for a more in-depth project like this

[spring geyser]

I mean, this whole discussion came from the point that I heard Cody basically say „would be nice if modules didn’t monkey patch stuff“ (paraphrasing, not a literal quote). This is basically just what we think would be required for that to happen.

Personally, I am not very opposed to monkey patching, as long as it’s done via libWrapper. I think it’s a viable solution to the problem, and honestly, I don’t see any big issues with it.

At this point, it’s a critical part of the module infrastructure. It’s installed on the majority of foundry installations, so by depending on it, modules usually aren't even adding any additional bloat, since it’s likely installed already.

To me it just feels like such a critical piece of infrastructure for the module ecosystem should become official at some point (or another, similar alternative)

[lucid crypt]

I'm gonna take this moment to move a bit more high level and philosophical because I'm seeing a lot of questions about "Why is Foundry against monkeypatching without offering a core way to accomplish the same thing?"

This statement may be contentious, but: The overlap of what you can possibly build and what Foundry will offer support for is fairly small in the grand scheme of things. That isn't to say you shouldn't build those things, but we can't offer support.

What does it mean for us to offer support, before someone burns me at the stake? Support specifically means "Foundry does its best to offer advice, code examples, deprecation periods if things break, migration guidance, and guarantees around when breaking changes occur in a Version"

We simply lack the time (and often, knowledge) to offer this kind of support for things outside of the Public API that we denote. We often struggle even to keep up on the existing Public API, but we are trying to grow it over time regardless to help provide more things that developers can use / build on safely.

To put this same concept in a different context: There are many packages out there that offer alternative ways to build Applications, such as VuePort. You can build some cool things with it, and some things can only be built if you use something like it, but Foundry cannot offer support for it. Because we cannot support it, we try to push developers, especially new ones, towards what we do support - Handlebars. Someone might claim "Foundry should offer a reactive framework in core then rather than recommend against the existing options", but it simply isn't feasible for us to build Core alternatives for every alternative thing the community dreams up.

Bringing it back to Monkeypatching, we can't offer support for it - it's not in the Public API. If you can only accomplish what you're building with Monkeypatching, then what you're building isn't supported. Again, do build it anyways if you feel comfortable being unsupported, but currently the only path to support is to request we expand the Public API, which we invite you to do.

I hope this helps clarify things here, please don't burn me in a glorious fire of wrath

[livid heath]

Well said @lucid crypt, I think you framed that well.

We can't simultaneously define the boundaries of the supported API and actually support that API while simultaneously offering "supported" pathways to veer out of bounds. If its out of bounds, its out of bounds - it's important for us to have those boundaries otherwise we would never get anything done except maintenance.

I think there is a practical downside of how easy it is to patch something in JS and how effective libWrapper is at providing an interface for doing that: we don't actually get that many API expansion requests. Certainly not as many requests as usages of libWrapper would imply might exist. I think most devs just wrap it and move on rather than surfacing their need for more flexibility back to us.

I'm not naive to think that we could quickly accommodate such requests if we got them, but the reality that JS lets you so easily work outside the mechanisms that we officially provide does become (to a degree) self-perpetuating.

[spring geyser]

Thanks for your explanations! Having read both of your messages, it still seems to me that there is a fundamental misunderstanding, and I believe you might be misjudging a bit why people use libWrapper.

Sure, there is some functionality that currently is hard to extend / adjust without using libWrapper / monkey patching (such as Canvas, as I already mentioned in the other thread). But I believe the main reason for modules to use libWrapper is the fact that it has been established as a de facto standard that it’s better for compatibility reasons.

While it is very common for systems, you will rarely see modules extending classes and setting them in the CONFIG, because in order to do so, you basically need to use the mixin pattern (that is if you care about being compatible with other modules and the system, instead of just overwriting what they did).

And in addition, you loose all the additional benefits you get from libWrapper, like conflict detection, allowing users to specify priorities, etc.

So let me iterate: this is not (only) about being able to adjust more functionality than is intended to be adjusted via the public API, it is much more about extending the core functionality in a more convenient and safe way.

The suggestion @gloomy geyser made wouldn't change at all what is supported to be extended / adjusted. It would just allow module authors to do it in a more convenient and safe way.

Sure, you could of course still override stuff marked as @private, but that’s not any different from what you can also do with the currently supported way of adjusting core functionality (extending classes and setting them in CONFIG).

On the flip side, I believe a very big part of the modules currently using libWrapper wouldn’t need to do so anymore, and could instead use the officially supported way of extending foundry (while also not even touching the private API in many cases).

Just to give an example:

In my module foreground-drawings, I am using libWrapper to wrap / monkeypatch a total of 5 methods in 4 different classes. But all of them are part of the public api. I am just using libWrapper because it’s more convenient and for the additionally safety regarding compatibility with other modules.

If foundry core provided a way to get those benefits, I would not have a need to monkey patch anything at all. And there is no need to increase the surface of what’s supposed to be extended at all for this.

(Well, not 100% correct, one of the classes is DrawingHUD, which as far as I understand is currently not configurable, so foundry core doesn’t provide a way to extend this class at the moment, aside from using its render hook. Please correct me if I'm wrong about this)

[gloomy geyser]

I repeat what ghost just said, he summarised what I believe is the reason people use libWrapper very well 🙂

If you install a bunch of libWrapper-using modules, and then look at the "Active Wrappers" panel, most of them are public API, and I believe the main reason those modules aren't extending classes but instead choose to use libWrapper is because otherwise compatibility between modules becomes a headache (since extension order then relies on module execution order). It's not that Core doesn't already officially support most of the extensions module devs are doing. It's that Core doesn't provide a way to do them in a way that scales to multiple modules...

I understand not providing support for modifying private members/fields, but I believe there should be an official way to extend public fields, similarly to what systems already are expected to do, but that would be scalable to multiple modules. Support would be offered for said "extension API" itself, as well as when people do certain supported things (e.g. extend public fields) with said API. If people extend private fields with this API, it makes sense they'd be on their own. This would be exactly the status quo when people override CONFIG classes, just without the compatibility aids for multiple modules.

[lucid crypt]

the methods being part of the Public API doesn't mean that monkeypatching them is supported - altering, wrapping, or replacing a Public API method outside of provided Hooks or extension points is not supported.

The public API is our guarantee that the method signature is stable and consistent. We promise we won't adjust it without a deprecation period, and we try our best to keep implementation stable as part of that. Modules that adjust the implementation break that promise for others who are using it, and therefore that behavior falls out of our support.

To phrase it as a support request, if someone came to us and said "I am trying to use this Draw method but it's not returning as the documentation says it should", and we discover in triage that another module has adjusted that method, we have to say "sorry, we can't help here, go reach out to that other author and try to work something out"

due to the lack of core conflict-resolution, there are many, many things that only a System or new-feature Module are supported to touch, and I think we're running into that here

[spring geyser]

I think you are still misunderstanding: @gloomy geyser's suggestion is about avoiding having to monkey patch at all. Instead, people could use the designated extension points that already exist now, just in a better way

[lucid crypt]

we don't have that right now, and we have no committed work to adjust how the current state is

[spring geyser]

Well, if there is stuff that is only intended to be extended by systems, it would be good to make that explicit. It’s the first time I hear about this.

[lucid crypt]

I agree we could make it better

[spring geyser]

And again, I don’t necessarily see a need to do anything about the situation at all. It's basically just a suggestion for how we could get people to move away from monkeypatching things and use the already existing designated extension points more

[lucid crypt]

here's a quick guide of my understanding:

• A system changing how initiative works: Supported
• A module adding new Applications for Sleep management that override the base Application class: Supported
• Having a single module installed that adjusts how Lighting works: Supported, probably
• Having multiple modules installed that adjust how Lighting works: Not Supported

[spring geyser]

Yes, that summarizes the current state, basically. Though you kind of can already make the last bullet point work already with the core extension points, it’s just a bit more cumbersome, and when things go wrong, it might be difficult for users to figure out what is going wrong. That’s where libWrapper comes in, solving both of these issues.

[gloomy geyser]

"more cumbersome" is putting it mildly 🙂
we're talking module authors adding hacks in each module to support each other's module, and things breaking on updates often

[lucid crypt]

"Can make work" and "supported / recommended" are not the same thing 😆

I do, idly, wonder how I've never had to use Libwrapper in my modules

I've built... a lot of things

I guess everything I build falls under "new, self-contained feature"

[spring geyser]

If supporting multiple modules adjusting the same / related functionality is an explicit non-goal, that’s also fine, I suppose, but I believe it severely limits what you can do in modules, at least if you want people to be able to use them in conjunction with other popular modules (or systems that change many things, like pf2e)

[lucid crypt]

all we ask if that if you stray from our supported paths that you don't complain / blame us when things break or go wrong

otherwise, god speed and we genuinely wish you the best of luck

[gloomy geyser]

but yes, the problem (and IMO improving this should be in the Foundry roadmap at some point, although not critical) is that not supporting "multiple modules extending the same thing" (i.e. for anything that e.g. Hooks aren't enough) effectively results in no module using the officially supported way of extending anything, since no module wants the headache of compatibility reports and being that module that doesn't work with others. It turns that officially supported path into one de facto only for systems (who are effectively "first-class" when it comes to Foundry, since they're the only ones guaranteed to have first go at extending things).

[spring geyser]

I think that’s a given 🙂

[lucid crypt]

for modules that are trying to adjust Core functionality, rather than add new self-contained functionality, I agree that there is a small amount of supported options there. We would like to make that better over time

[lucid crypt]

if I could summarize this convo, it'd be: "Just because Foundry does not officially support / recommend X, doesn't mean we're telling you not to"

We recommend you don't eat ice cream, as it's bad for you. We also love ice cream, and still eat it from time to time, and you're free to as well.

[spring geyser]

I believe, if you want to improve that at some point, taking a look at ruipin's suggested solution might be a good idea. I also think it’s not critical at all since we have libWrapper and can rely on it. But if you want to make it unnecessary (while not increasing the amount of stuff that people are supposed to extend), the suggestion is a pretty good starting point, in my opinion.

[gloomy geyser]

fwiw, my only "complaint" if anything comes even close to that term are the statements about wishing people didn't monkeypatch, since they feel somewhat dismissive of the reasons why people monkeypatch, given there is no alternative for many use cases. I don't complain that there's no support for monkeypatching (though believe improving this should be in the roadmap somewhere) and know I am in my own when I do it. Heck, libWrapper was basically me on my own, trying to make it so that people weren't "on their own" as much.

[gloomy geyser]

must confess I haven't looked at your modules (though I probably use some, for all I know), but there is a decently big class of modules that doesn't need monkeypatching. Mostly, if you're a "reactive" module (i.e. mostly react to events, enhancing behaviour rather than altering it), you can rely on hooks for most if not all functionality. It's more when you want to deeply integrate into existing code that zero-monkeypatching becomes more difficult if not impossible.

[lucid crypt]

I try to always use that statement with the tack on of "please let us know how we could expand the public api to support what you want to do so we can get you to supported paths"

You've given some good thoughts on a general path on how we may do that, and although V10 is currently full, maybe in V11 we can approach it

a Gitlab issue summarizing it would help

[gloomy geyser]

if I have time I might write one, but tbf could probably just link to this thread lol

[spring geyser]

Probably still makes sense to condense it into an issue. If you want help with that, let me know

[gloomy geyser]

also, remembered I wanted to mention this, related to Atropos' previous statement:

I think most devs just wrap it and move on rather than surfacing their need for more flexibility back to us.
This is the actually one of the reasons I added the "Active Wrappers" tab to libWrapper, to surface what modules are actually patching.
It would be an interesting exercise to install a bunch of popular libWrapper modules and seeing what things they are patching, and whether these could have better public APIs / Hooks.

(although without a more formalised module alternative to libWrapper, I suspect many of them might know about public APIs and don't use them on purpose, due to the compatibility downsides of extending classes as a module)

(alternatively, a script to go through the module manifests, download source code from e.g. github, and extracting the second parameter from any found libWrapper.register calls should also be relatively simple and could also be pretty effective)

[shy cipher]

A lot of "bad practices" of monkeypatching (among many other things) come from other people doing it.
This is worsened with the general recommendation to look at other modules in how they did things to solve such problems.
Also, monkeypatch solutions are for the most part actually much more readable (just the target method overridden, no extra cruft, no need to register some class you made for holding it, etc.) than other solutions, and look much more self-contained, which increases their appeal.

[gloomy geyser]

fwiw, I don't think there's a big difference in cruft/readability between e.g.

registerMixin("some-module", Drawing, (parent) => class SomeModuleDrawing extends parent {
    get defaultOptions() {
      const res = super.defaultOptions;
      res.height += 70;
      return res;
    });
  }, {type: CONSTS.WRAPPER});``` and
```js
libWrapper.register("some-module", "Drawing.prototype.defaultOptions", function(wrapped) {
  const res = wrapped();
  res.height += 70;
  return res;
}, libWrapper.WRAPPER);```

the former would be cleaner assuming you're doing multiple wrappers to the same class at once, and has the advantage that it doesn't require some weird "prototype" thing that usually confuses people, it's explicit that you're overriding a getter, and it relies on JS's "super" keyword so feels more natural than a parameter.
Also would probably perform better.

[spring geyser]

I think it’s just not that well known… it simply hasn’t been established (while libWrapper has)

[livid heath]

I shudder to think of having 50 different modules all registering a mixin which creates a massive inheritance chain for certain prototypes

[spring geyser]

50 sure would be excessive… but it’s kind of the same when multiple modules wrap the same method with libWrapper. But you do have the benefit that it’s per method instead of per class

[livid heath]

I don't know what the exact implications would be of having an excessively long prototype change, but it definitely sounds painful

probably is not that painful in reality

[spring geyser]

Maybe we can gather some insights as to how many modules currently wrap the same methods / classes, to see how bad it actually is

[livid heath]

backwards-looking data about how bad it is today isn't necessarily the sole consideration for identifying the best future design

I think we have to imagine a post-apocalyptic future where 250+ active modules is the norm and ask ourselves what would happen then.

[spring geyser]

That sounds like the apocalypse very much is still ongoing 😄

[gloomy geyser]

For whatever it is worth, as long as you aren't constantly changing the prototype chains (which you wouldn't be), any modern JS engine should JIT prototype lookups into something basically instantaneous

For example, the following piece of code which sets up a property chain 1000 classes long, which only overrides b() but not a() from the original class:

// Utility function
function measure_perf(name, fn, reps=1000) {
    const results = [];
    for(let i = 0; i < reps; i++) {
        const start = performance.now();

        fn();

        const end = performance.now();
        results.push(end - start);
    }

    const sum = results.reduce((a, b) => a + b, 0);
    const avg = sum / results.length;

    const display = Math.ceil(avg * 100) / 100

    console.log(`${name}: ${display}ms`);
}


// Create long prototype chain
class Orig {
    a() { return 1 };
    b() { return 1 };
};

let Last = Orig;
for(i=0; i < 1000; i++) {
    Last = class extends Last {
        b() { return super.b() + 1 }
    };
}


// Measure performance of calling a
measure_perf("Orig.a()", ()=>{ let obj = new Orig(); console.log(obj.a()) });
measure_perf("Last.a()", ()=>{ let obj = new Last(); console.log(obj.a()) });


// Measure performance of calling b
measure_perf("Orig.b()", ()=>{ let obj = new Orig(); console.log(obj.b()) });
measure_perf("Last.b()", ()=>{ let obj = new Last(); console.log(obj.b()) });```
results (on my computer, Windows with latest Firefox) in ```js
Orig.a(): 0.05ms
Last.a(): 0.08ms
Orig.b(): 0.03ms
Last.b(): 0.12ms```
So a 1000-long prototype chain is only slightly slower than no prototype chain.

Chrome is even faster:

Orig.a(): 0.02ms
Last.a(): 0.03ms
Orig.b(): 0.02ms
Last.b(): 0.09ms

If you remove the construction from the loop, it makes essentially zero difference how many prototypes you have in the chain (which confirms my hypothesis that the engine is optimising this actively, probably at construction or on function call).

const orig = new Orig();
const last = new Last();

// Measure performance of calling a
measure_perf("Orig.a()", ()=>{ console.log(orig.a()) });
measure_perf("Last.a()", ()=>{ console.log(last.a()) });

// Measure performance of calling b
measure_perf("Orig.b()", ()=>{ console.log(orig.b()) });
measure_perf("Last.b()", ()=>{ console.log(last.b()) });```
Firefox:

Orig.a(): 0.05ms
Last.a(): 0.04ms
Orig.b(): 0.04ms
Last.b(): 0.08ms```
Chrome:

Orig.a(): 0.02ms
Last.a(): 0.02ms
Orig.b(): 0.02ms
Last.b(): 0.08ms```

If you mean it'd be messy in terms of debugging, then yeah, long prototype chains could be annoying, but that's not too different from having long libWrapper / monkeypatching chains right now. And I'd argue it might be a better situation, since there's no "magic" going on hidden behind the libWrapper blackbox. It's all made up of "normal" JS prototype chains. It also helps that the JIT can actually do its job -- something that libWrapper makes more difficult.

[gloomy geyser]

fwiw, even disregarding the results above, even if you have 250 modules, it is doubtful you'd ever have 250 modules registering mixins for the exact same classes.
(I'm assuming, like libWrapper, you'd limit each package to a single Mixin for each individual class)