ractivejs / ractive

Next-generation DOM manipulation
http://ractive.js.org
MIT License
5.94k stars 396 forks source link

[Question] Since 1.4.0-56, lifecycle event methods in child classes have to be public #3385

Closed seijikun closed 3 years ago

seijikun commented 3 years ago

Description:

When using Ractive with Typescript: Since Version 1.4.0-build-56, the lifecycle event methods such as onrender(), oncomplete(), onconfig(), onunrender() ... have to be public in subclasses of Ractive, otherwise the project will not compile. Was this change on purpose?

Versions affected:

Reproduction:

Before:

export class MyComponent extends Ractive {
    constructor(opts?: InitOpts) { super(opts); }

    protected onrender() {
        // do stuff
    }
}

this now fails with:

Class 'MyComponent' incorrectly extends base class 'Ractive<Ractive<any>>'.
  Property 'onrender' is protected in type 'MyComponent' but public in type 'Ractive<Ractive<any>>'.ts(2415)

to fix it, the protected access specifier has to be relaxed to public. To me, this does not seem right, since that event method now is part of Ractive's public API, and can be called from outside (users of a Component or Ractive instance).

evs-chris commented 3 years ago

Sorry, I missed this issue. Yes, those should probably be set to protected, though they aren't the preferred way to add lifecycle hooks. They recent got added to the typings, so before they were not part of the interface. I think you can set a less restrictive access modifier on a child class, so protected shouldn't affect anything that is already declared public, but I might be thinking of any number of other languages here.

seijikun commented 3 years ago

I think you can set a less restrictive access modifier on a child class, so protected shouldn't affect anything that is already declared public, but I might be thinking of any number of other languages here.

I'm afraid I don't completely get what you mean by that. Isn't that exactly what I did? MyComponent being a child class of Ractive and using protected on the onrender() method, instead of public, like in the parent class. This causes a compile error. Or did you mean a change needed in Ractive itself?

evs-chris commented 3 years ago

I mean, if we change the ractive typings to have lifecycle methods be protected, it wouldn't stop someone from exposing them if they wanted to? I don't think they were intended to be exposed as methods at all, so it probably wouldn't be awful to restrict them. It only really makes sense to set them in the init options if you're not using a class anyways.

seijikun commented 3 years ago

Ah, yes. That makes sense. Relaxing the access specifiers in a child class seems to work. demo

evs-chris commented 3 years ago

Once travis finishes travising, the lifecycle methods should be protected on edge.

seijikun commented 3 years ago

Thanks! No compile errors left ;)