DEV: Add compatibility with the Glimmer post menu

[megothss]

Requires discourse/discourse#28670

[davidtaylorhq]

We'll need to add the comment so that ember-template-lint ignores this

(it's not worth us spending the time to update <MountWidget, since it'll hopefully be removed in the next few months)

[megothss]

I added the comments

[davidtaylorhq]

I would expect this to fail? In static methods, this refers to the Class itself (not the instance), so it won't have an owner 🤔

[CvX]

btw, I did want to set the owner on context arg, so that this would work:

static shouldRender(args, context) {
  getOwner(context)

I think I have this core change stashed somewhere 😅

we could make this work too/instead but using context is less surprising, I think? (though we use getOwner(this) almost anywhere so I see why having the same "incantation" here could be nice)

[davidtaylorhq]

Yup agreed - adding the owner on context and/or args makes sense. I'd prefer that to doing anything unusual with this.. (I don't think we should be setting owners on Klass objects - they persist beyond the lifetime of the ApplicationInstance)

BTW, worth noting that this particular case, it's not a plugin outlet. We're just using a similar shouldRender pattern for the new post-menu. But still, we should aim for the same ownership approach for this and for plugin outlets 💯

[CvX]

BTW, worth noting that this particular case, it's not a plugin outlet.

Ah, missed that fact 😃

I don't think we should be setting owners on Klass objects - they persist beyond the lifetime of the ApplicationInstance

I wouldn't be setting owner on a class itself but doing something like:

setOwner(context, owner);
shouldRender.call(context, args, context);

// vs `shouldRender(args, context);`
// which btw, means `this` currently is `window` I think?

[davidtaylorhq]

// which btw, means this currently is window I think?

In a static method, this. refers to the class itself. So you can do stuff like

class Blah {
  static shouldRender(){
    return this.someMethod();
  }
  static someMethod(){
    return true;
  }
}

so if we use .call/.bind/.apply to change the this. value, it'll break this fundamental functionality of static methods.

[CvX]

I meant it in the plugin outlet context. This is the way we call shouldRender:

https://github.com/discourse/discourse/blob/main/app/assets/javascripts/discourse/app/lib/plugin-connectors.js#L229-L230

[davidtaylorhq]

Ahhh.... Then I think we should fix is so it works as a normal static method 😅

[megothss]

The API is now receiving the owner as an explicit parameter and the variables match the (args, context, owner) signature for consistency

[davidtaylorhq]

Cool! We should make sure to document this pattern.

Does silencing the deprecation also stop it from affecting the 'glimmer_post_menu_mode=auto' system?

We also need to think about how we'll notify developers to remove the old logic, once the widget menu is removed. I guess at that point, we could introduce a new deprecation with a different name?

[megothss]

Yep. Silencing the deprecations stops them triggering the legacy mode.

I guess introducing a new deprecation may be a good idea on this case. Or we can log an error message in the console like we did in the header.

[davidtaylorhq]

Gotta think about what will happen to this logic after we remove the use_glimmer_post_menu boolean in future 🤔

Could we just register the transformer unconditionally? It'll be a no-op if the user/site is still using the widget implementation?

[megothss]

This logic is not needed anymore.

This was from an earlier commit of the API