Consider allowing registering a wrapper on any object

[LukeAbby]

Is your feature request related to a problem, e.g. with a module? Please describe.
lib-wrapper currently assumes that what you're wrapping can be found in the global scope. This can be a problem for two reasons:

1. The reason I'm actually filing this issue for which is that it makes TypeScript types a bit clunkier. One quirk of TypeScript is that typeof globalThis doesn't intrinsically include all globals and so GetProperty<typeof globalThis, "some-key"> is difficult to make work nicely.
2. In general I've seen some annoyance that it can be difficult to patch non-global objects or objects with special characters. lib-wrapper does have a "DSL" mentioned in the documentation to make this somewhat more possible like "CONFIG.Actor.sheetClasses.character["dnd5e.ActorSheet5eCharacter"].cls.prototype._onLongRest" (scare quotes over "DSL" since it really is meant to be as close to 1:1 to JS as reasonable) but this isn't an entirely perfect solution.

Describe the solution you'd like
It would be nice to be able to do something like this:

const api = game.settings.get("other-package").api;

libWrapper.register("some-package", { target: api, key: "some-method" }, fn, type, options);

Describe alternatives you've considered
Continue allowing only the current calls of register. The fact that only globals can be overriden may be an intentional design decision of lib-wrapper.

Additional context
N/A

[ruipin]

This is something that I have considered adding before, but could never think of a very reasonable way to do so.

From a purely technical standpoint, the libWrapper wrapper code does support wrapping arbitrary Javascript objects. The requirement for things to be reachable from the global scope is purely artificial and explicitly enforced by the API I designed.

There are multiple issues here that had me hold back from improving the API, but it mostly has to do with the way the API is designed.

The API assumes that every method has relatively unique names, and uses the target "DSL" as you say both as a way to access the object but also to identify that method. This name is used when presenting things to the user, in hooks, in error messages, as well as for the ignore_conflicts methods.

Without enforcing things to be globally accessible, someone could call the same object "foo" and someone else "bar" and neither would be easily identifiable for the user. I could not think of a way to uniquely identify wrappers in human-readable format without relying on names supplied by users of the library. Names supplied by the module writers are fine for a single module, but can quickly become undecipherable once you have 3+ modules wrapping the same method using different names.

Yes, module writers could assign things to global scope, wrap them, then unassign, but that requires extra effort. The API as it stands guides users to something that is decipherable and manageable.

Now, I am not against allowing to specify the target object directly, but I am concerned about module writers not using decipherable keys and that cascading throughout the libWrapper UX - and even affecting other modules.

Your API suggestion (target and key object) is sensible (and probably exactly what I'd go for), but doesn't really solve this particular issue. And at least in the example you gave it seems relatively simple to access that object from the global scope, at least from normal JavaScript, and then most of these issues would simply not be a concern.

As for special characters, I thought that was already possible given the "DSL" as you call it? Can you give an example of where special characters are problematic?

However, I am not very familiar with TypeScript so can't say I've put much thought as to whether there is anything we could do to improve the TS experience. Could you describe the issues facing TS in more detail? I'd also be open to suggestions here.

[LukeAbby]

The API assumes that every method has relatively unique names, and uses the target "DSL" as you say both as a way to access the object but also to identify that method. This name is used when presenting things to the user, in hooks, in error messages, as well as for the ignore_conflicts methods.

Perhaps an explicit name: "..." would be warranted, i.e. { name: "some-module-api", target: api, key: "some-method" } would be used to identify it. So some-module.some-module-api wrapping some-method. Admittedly this isn't perfect.

As for special characters, I thought that was already possible given the "DSL" as you call it? Can you give an example of where special characters are problematic?

Ah, sorry, I should have phrased this less ambiguously. As far as I know the DSL should let you manage any special characters but I just meant having to pay attention to quoting rules and knowing the DSL exists. Granted you have to know { target: ..., key: ... } exists as well.

However, I am not very familiar with TypeScript so can't say I've put much thought as to whether there is anything we could do to improve the TS experience. Could you describe the issues facing TS in more detail? I'd also be open to suggestions here.

Basically strongly typing this API out of the box can be a bit of a lift. A lot can be solved on the TS end, this is the basic chassis:

declare function simpleRegister<K extends keyof typeof globalThis>(key: K, callback: (wrapped: typeof globalThis[K]) => void): void;

I do plan to add my best support for this version but there's 3 issues:

1. Not all globals appear in typeof globalThis. I can fix this in fvtt-types itself which honestly is probably good enough for the average user.
2. Computing all possible dotkeys ahead of time is technically possible (ignoring recursive objects that could be pruned). However that union would be massive containing variants like a.b.c vs a["b"].c vs a.b["c"] vs a["b"]["c"] and so on. I can work around this with a validator type (basically validating the string more lazily rather than greedily computing all possibilities ahead of time) but it's something you have to be careful about doing because while conceptually easy you need autocompletion to still work and if you're not careful it's laggy. Realistically computing the union of all possible keys would be laggy too though.
3. It can be hard to work with register in a generic context. Let's say you had this JS code:

function exampleWrapper(key) {
  libWrapper.register(MODULE_ID, key, (wrapped, a, b, c) => {
    // Do stuff
    return wrapped(a, b, c);
  });
}

If you wanted to make exampleWrapper type safe you'd have to write something like this TypeScript code:

// The type `KeyForFunctionWithThreeArguments` would validate that `K` is a valid lib wrapper key _and_ that key leads to a function with 3 arguments.
function exampleWrapper<K extends string>(key: KeyForFunctionWithThreeArguments<K>) {
  libWrapper.register(MODULE_ID, key, (wrapped, a, b, c) => {
    // Uh oh, this is going to error anyways because the type of `libWrapper.register` doesn't realize what `exampleWrapper` is trying to do is sound.
    return wrapped(a, b, c);
  });
}

Note: This is more a problem because of issue 2, that keys can't be computed ahead of time. If all possible keys could be known then the signature could be exampleWrapper(key: KeysForFunctionsWithThreeArguments). This would be enough to typecheck the body successfully.

However even though both signatures will have the same end effect for callers, essentially the signature exampleWrapper<K extends string>(key: KeyForFunctionWithThreeArguments<K>) would have to have fairly sophisticated validation logic. Sophisticated enough that it may as well be a black box and so libWrapper.register will really only know that it's going to be passed a string (because of K extends string). Therefore it won't be able to type wrapped as (a: unknown, b: unknown, c: unknown) => unknown, making the body.

Realistically most TypeScript authors would just give up type safety here or make a substantial refactor. If instead this new signature was available you could write:

function exampleWrapper<Target extends object>(target: Target, key: PropertiesOfType<Target, (a: unknown, b: unknown, c: unknown) => unknown>) {
  libWrapper.register(MODULE_ID, { target, key }, (wrapped, a, b, c) => {
    // Should work!
    return wrapped(a, b, c);
  });
}

This does require a change but a much less substantial one. You can think of PropertiesOfType<Target, (a: unknown, b: unknown, c: unknown) => unknown> as analagous to the earlier KeysForFunctionsWithThreeArguments; it gets all properties that lead to that are functions with 3 arguments. However being flat (i.e. disallowing sub-properties like a.b.c) it's very easy to write.

Note that I've skipped over some subtleties, for example PropertiesOfType is not built into TypeScript but is in fvtt-types and is easy to write compared to KeyForFunctionWithThreeArguments. I've also assumed that { target: obj, key: k } would interpret keys like a.b.c as obj["a.b.c"].

[ruipin]

Perhaps an explicit name: "..." would be warranted, i.e. { name: "some-module-api", target: api, key: "some-method" } would be used to identify it. So some-module.some-module-api wrapping some-method. Admittedly this isn't perfect.

I don't think separate name/key is necessary here, I'd probably just have a single string parameter as identifier (e.g. name to make it more obvious it is user facing). This parameter would be collected into a list (since each module might use a different name compared to other modules) and then the UI would somehow present all those names provided to the user, e.g. error messages could show

some-module: Foo API
another-module: Bar API

if there were two modules wrapping the same API but provided different names.

The main issue is there's some places in the UI where there is no real space for multiple names, and I'm not sure how to pick the "better" one. I suspect I just have to pick one at random (e.g. the first one).

Basically strongly typing this API out of the box can be a bit of a lift. A lot can be solved on the TS end, this is the basic chassis:

Thanks, I think I kind of get it now, even if I am still missing plenty of details due to my lack of familiarity with TS.

[LukeAbby]

I guess another issue would be if one module wraps game.modules.get("another-package").api.a.b.c and another wraps game.modules.get("another-package").api.d.e.f; to a human these two objects are pretty related but to lib-wrapper it's unclear how these could be linked.

I think explicit names are probably enough for the 80% case, expecting users to consolidate on some consistent name somehow seems unreasonable to me.

Up to you if you want to add this of course.

[ruipin]

Question: As far as making it easier for TS to use the libWrapper API, would it be helpful to require the object to be reachable from global scope but also allow a specific instance to be passed it?

For example, instead of

libWrapper.register("some-package", "game.settings[\"another-package\"].api.some-method", fn, type, options);

the API could easily accept

const api = game.settings.get("other-package").api;
libWrapper.register("some-package", {
  target: api, method: "some-method", scope: "game.settings[\"another-package\"].api"
}, fn, type, options);

or something similar, and still enforce consistent naming and reachability from global scope?

I am considering whether there is a middle-ground here that I could quickly and easily implement that would already make things much easier for you, while I figure out how to allow unscoped objects.

[LukeAbby]

It would make a things a lot easier, yeah! No rush though, I do plan to write types for the string version as well since I know some authors won't bump their lib-wrapper dependency. I'm writing the types to publish for the community as a whole rather than just myself.