Page MenuHomePhabricator

LSP violations in OOUI
Open, Needs TriagePublic

Description

When I was writing type declarations of OOUI I found out that some OOUI classes are designed in violation of Liskov substitution principle, which is forbidden in TypeScript:

  • OO.ui.mixin.GroupElement (Changes method signatures of OO.EmitterList)
  • OO.ui.CheckboxInputWidget (Change signatures of change event)
  • OO.ui.NumberInputWidget (Changes method signatures of OO.ui.TextInputWidget)
  • OO.ui.OutlineControlsWidget (Change signatures of move and remove event)
  • OO.ui.SelectFileWidget (Change signatures of change event and method signatures of SelectFileInputWidget)
  • OO.ui.SelectWidget (Change signatures of move and remove event)

This makes TypeScript adoption difficult, as we have to write things like this:

// Error: Type 'NumberInputWidget' is not assignable to type 'TextInputWidget'...
let widget: OO.ui.TextInputWidget = new OO.ui.NumberInputWidget();

// Use type assertion as a fix
let widget: OO.ui.TextInputWidget = new OO.ui.NumberInputWidget() as unknown as OO.ui.TextInputWidget;

See also: T107639

Event Timeline

  • OO.ui.OutlineControlsWidget (Change signatures of move and remove event)
  • OO.ui.SelectWidget (Change signatures of move and remove event)

These two look like obvious but fixable mistakes. Note that the events are not replacing inherited ones, but instead both sets of events (with different signatures) can be emitted. If any code actually relied on them, it would probably throw exceptions immediately. So while fixing this would technically be a breaking change, it seems unlikely to break any real code.

OutlineControlsWidget probably shouldn't mix in GroupElement (that's where it gets the conflicting events), as that functionality is apparently never used by anyone. VisualEditor uses the OutlineControlsWidget events.

SelectWidget probably shouldn't emit the events, as it's just a worse version of the GroupElement events. It seems that no one uses them.

  • OO.ui.mixin.GroupElement (Changes method signatures of OO.EmitterList)

To clarify, the LSP violation is only in the type of parameters / return values? Technically you're right, but I've always understood this relationship more like a generic specialization than inheritance. That is:

  • OO.EmitterList can hold a list of any OO.EventEmitter
  • OO.ui.mixin.GroupElement narrows that down to only OO.EventEmitter that are also OO.ui.Element
  • OO.ui.MenuSelectWidget narrows that down further to only OO.ui.MenuOptionWidget

Hopefully that makes sense and there is a better way to document it.

  • OO.ui.CheckboxInputWidget (Change signatures of change event)
  • OO.ui.NumberInputWidget (Changes method signatures of OO.ui.TextInputWidget)

Despite technically using inheritance, these are not supposed to be substitutable. If you look at other InputWidget subclasses, you'll find further horrors, e.g. CheckboxMultiselectInputWidget. Inheritance here is just a hack to allow code reuse, and it may be better to pretend that these classes are not related.

  • OO.ui.SelectFileWidget (Change signatures of change event and method signatures of SelectFileInputWidget)

This one is a right mess.

Sorry for the late reply.

  • OO.ui.mixin.GroupElement (Changes method signatures of OO.EmitterList)

To clarify, the LSP violation is only in the type of parameters / return values? Technically you're right, but I've always understood this relationship more like a generic specialization than inheritance. That is:

  • OO.EmitterList can hold a list of any OO.EventEmitter
  • OO.ui.mixin.GroupElement narrows that down to only OO.EventEmitter that are also OO.ui.Element
  • OO.ui.MenuSelectWidget narrows that down further to only OO.ui.MenuOptionWidget

Hopefully that makes sense and there is a better way to document it.

The problem here is OO.ui.Element doesn't mixin OO.EventEmitter, but OO.ui.Widget does.

  • OO.ui.CheckboxInputWidget (Change signatures of change event)
  • OO.ui.NumberInputWidget (Changes method signatures of OO.ui.TextInputWidget)

Despite technically using inheritance, these are not supposed to be substitutable. If you look at other InputWidget subclasses, you'll find further horrors, e.g. CheckboxMultiselectInputWidget. Inheritance here is just a hack to allow code reuse, and it may be better to pretend that these classes are not related.

Then something should be changed in the documentation. TypeScript handles inheritance seriously and current implementation makes it incompatible with TypeScript.

  • OO.ui.SelectFileWidget (Change signatures of change event and method signatures of SelectFileInputWidget)

This one is a right mess.

And now it has been merged into SelectFileInputWidget so the chaos continues.


BTW, it looks like OOjs's inheritance model (OO.inheritClass and the static field) is not compatible with both ES6 classes and TypeScript, thus it makes the effect of TS definitions quite limited. Are there any plans to change this in the future?

BTW, it looks like OOjs's inheritance model (OO.inheritClass and the static field) is not compatible with both ES6 classes and TypeScript, thus it makes the effect of TS definitions quite limited. Are there any plans to change this in the future?

It is fully compatible with ES6 classes. I experimented with using ES6 classes for custom OOUI widgets a while ago: https://gitlab.wikimedia.org/repos/ci-tools/patchdemo/-/merge_requests/561 and I also found an example that is actually deployed in production on Wikimedia wikis: https://gerrit.wikimedia.org/g/mediawiki/extensions/TimedMediaHandler/+/fe048dff4a7623d0d104fd897f0ddedf5792f1b3/resources/ext.tmh.player.dialog.js#23

We used to have a plan to move static properties to the class itself, instead of having them in .static: T89721. I think it's still possible to do, although it seems there wasn't much interest in doing it recently.