Add ObserverMixin to ModelBase typings

[bajtos]

See #1820 and loopbackio/loopback-next#4730.

Model APIs

• ModelBaseClass
• PersistedModelClass
• typeof KeyValueModel

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• Commit messages are following our guidelines

[bajtos]

This is actually a breaking change.

Before, a value of type typeof PersistedModel was assignable to PersistedModelClass.

With my change in place, this is no longer possible, because typeof PersistedModel is missing ObserverMixin methods.

[bajtos]

This is actually a breaking change.

Before, a value of type typeof PersistedModel was assignable to PersistedModelClass.

With my change in place, this is no longer possible, because typeof PersistedModel is missing ObserverMixin methods.

As I was thinking about this problem, I came to the conclusion that we should prefer ease of use and ergonomics of our typings over implementation simplicity.

The original implementation in fff92b8 was based on changing {Model}Class type aliases to include & ObserverMixin. While this is quick fix in juggler that can reuse typings in ObserverMixin, it introduces a breaking change because typeof PersistedModel is no longer assigne-able to PersistedModelClass, which I would find very surprising as a user.

I have reworked this pull request to simply copy typings of the public members of ObserverMixin into ModelBase class as static members. This is not perfect because we are duplicating typings, but it should be find in practice, because the API is pretty much stable by now.

I have verified that @loopback/next compiles cleanly against the updated typings, therefore I believe this new version should be fully backwards-compatible 🎉

The code here is prepared to support better-typed Operation Hook Context introduced in #1820 🚀

I consider this PR as ready for review. @emonddr @zbarbuto @jannyHou @dhmlau PTAL.

/cc @hacksparrow @raymondfeng - I'd like to hear your opinions too.

[hacksparrow]

@bajtos there is at least one place where PersistedModel & ObserverMixin is used instead of typeof PersistedModel & ObserverMixin. Would that affect anything in the PR?

[bajtos]

there is at least one place where PersistedModel & ObserverMixin is used instead of typeof PersistedModel & ObserverMixin. Would that affect anything in the PR?

As I understand the API, PersistedModel & ObserverMixin is incorrect. There are no observer-related methods on model instance, those methods are always static.

I think you need to fix those places and modify PersistedModel & ObserverMixin to typeof PersistedModel & ObserverMixin

[bajtos]

AFAICT, your PR loopbackio/loopback-next#4730 uses the following two variants, both of them are correct:

• typeof PersistedModel & ObserverMixin
• juggler.PersistedModelClass & juggler.ObserverMixin

Notice that in the second case, we are using PersistedModelClass.

[emonddr]

Please add a comment that ModelBaseClass is an alias of typeof ModelBase and we are testing the alias here. thx.

[bajtos]

See 6c38347, is my comment good enough?

[emonddr]

The comment is great. thx. But a tiny typo: verifying that the value retunred

[bajtos]

Rebased on top of #1820, added few more tweaks in 6c38347.

[bajtos]

I linked my modified version of juggler into packages/repository of loopback-next and run npm run build to verify that my changes are not breaking anything. I did this for both master and feat/operation-hooks (loopbackio/loopback-next#4730) branches.

[bajtos]

Created #1825 to fast-track landing of Listener fix.

[bajtos]

Cross-posting from Slack.

More of a general TS question:
Why do we do removeObserver<T extends typeof ModelBase> and not removeObserver<T extends ModelBase>?
What does this: T do and why do we need to do that?

Please start here:

  // See also https://github.com/microsoft/TypeScript/issues/5863#issuecomment-410887254
  // for more information about using `this` in static members.

Here is what we want to describe: for a model extending from BaseModel - let's say Product, when the static method removeObserver is invoked, the context argument is of type OperationHookContext<Product>.

If removeObserver was a prototype method, then we could use OperationHookContext<this>. Because it's a static method, we need to find a different solution - see microsoft/TypeScript#5863 (comment)

Regarding this: T, see this docs for introduction to how this is handled: https://www.typescriptlang.org/docs/handbook/functions.html#this
this parameters (function arguments) are described later in that doc, see https://www.typescriptlang.org/docs/handbook/functions.html#this-parameters

You can provide an explicit this parameter. this parameters are fake parameters that come first in the parameter list of a function.

For example, a mocha test function returning a promise is typed as function (this: Mocha.Context): Promise<void>, which allows you to access API like this.timeout() and this.skip() inside that function.

Now in our case, by using this: T, we allow the compiler to infer what's the target model class we are going to observe, so that can describe the type of context argument using the target model class.

Why do we do removeObserver<T extends typeof ModelBase> and not removeObserver<T extends ModelBase>

Because observer API is mixed into the class constructor (Product.observe()), not the prototype (new Product().observer()).

[bajtos]

Added b01cd74 to capture some of the information from my comment above.