telepath icon indicating copy to clipboard operation
telepath copied to clipboard

Ability to cleanly extend media definitions when subclassing an adapter

Open gasman opened this issue 4 years ago • 2 comments

The discussion at https://github.com/wagtail/wagtail/pull/7094#issuecomment-827547708 shows a few iterations on extending one of the existing adapters supplied with Wagtail, on both the Python and Javascript side. This means appending the .js file containing the new code to the media definition on the parent, and the natural way to do that would be:

class ImageBlockAdapter(StructBlockAdapter):
    js_constructor = 'bakerydemo.blocks.ImageBlock'

    @cached_property
    def media(self):
        return super().media + forms.Media(js=[
            static('js/image-block.js')
        ])

but adding two media objects together like that doesn't guarantee ordering (because it performs more complex dependency resolution, and we haven't specified that StructBlockAdapter's media is a dependency of image-block.js). We ended up having to either repeat the parent's js, or poke inside its internal _js property.

The solution is probably to keep those definitions as plain lists for as long as possible, add a bit of (metaclass?) hackery so that we append to those lists on subclassing, and only turn them into Media objects at the point where telepath retrieves the media property.

gasman avatar May 13 '21 20:05 gasman

Django has a nice-in-theory mechanism for inheritance when defining media through an inner Media class: https://docs.djangoproject.com/en/3.2/topics/forms/media/#extend. It would be nice to adopt that pattern or something close to it.

We can't use Django's implementation directly, for two reasons:

  • Wagtail's media definitions need to use a dynamic property so that we can use the versioned_static helper inside it without evaluating it on load (https://github.com/wagtail/wagtail/issues/5632); once a dynamic property is use, static media definitions and inheritance don't get a look in
  • The combined media definition suffers from the same non-deterministic ordering issues as described above:

(on Django 3.0.14)

>>> from django.forms import Media, Widget
>>> class ChooserWidget(Widget):
...     class Media:
...         js = ['jquery.js', 'chooser.js']
...
>>> class ImageChooserWidget(ChooserWidget):
...     class Media:
...         js = ['image-chooser.js']
...
>>> w = ImageChooserWidget()
>>> w.media
Media(css={}, js=['jquery.js', 'image-chooser.js', 'chooser.js'])

We might expect the ChooserWidget -> ImageChooserWidget inheritance to imply that image-chooser.js depends on chooser.js, but it doesn't - it just uses the ordinary 'merge' operation on the two media objects, which requires both sides to specify their dependencies explicitly. In other words, ImageChooserWidget's media has to explicitly include chooser.js, which rules out the kind of 'black box' treatment of ChooserWidget that telepath's way of thinking really demands: "ImageChooserWidget's media consists of whatever ChooserWidget needs, plus image-chooser.js".

I think this is a misfeature on Django's side: a naive merge algorithm that just repeatedly runs "if item not in result: result.append(item)" would be better on all counts except dubious edge cases like [A, B, D] + [A, C, D] having to equal [A, B, C, D] and not [A, B, D, C]. Given that telepath doesn't have the obligation that Django has to preserve backward compatibility here, I reckon that reinventing the wheel and rolling our own alternative is the right technical approach :-)

gasman avatar May 17 '21 16:05 gasman

Following the changes in #4, where the favoured way of including media is now to call add_media within the pack / telepath_pack method, we're no longer reliant on inheritance rules, and there's little reason for anyone to work with Media objects directly any more. Right now we still use them as our canonical representation within add_media, so it's still possible to get unintuitive results like the above from repeated calls to add_media - but that could now be replaced with the simpler append-based rule.

gasman avatar Jun 01 '21 17:06 gasman