Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to cleanly extend media definitions when subclassing an adapter #5

Open
gasman opened this issue May 13, 2021 · 2 comments
Open

Comments

@gasman
Copy link
Contributor

gasman commented May 13, 2021

The discussion at wagtail/wagtail#7094 (comment) 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
Copy link
Contributor Author

gasman commented May 17, 2021

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 (collectstatic with ManifestStaticFilesStorage fails on 2.7rc1 wagtail#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
Copy link
Contributor Author

gasman commented Jun 1, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant