Issues overriding getters in a controller (3.0.0)

[merefield (Robert)]

I’ve got this code in my initializer, it’s attempting to override the getter for allAdminRoutes:

    api.modifyClass('controller:admin-plugins', {
      pluginId: PLUGIN_ID,

      get allAdminRoutes() {
        return this.model
          .filter((p) => p?.enabled)
          .map((p) => {
            return p.admin_route;
          })
          .filter(Boolean);
      },
    });

It’s failing with:

TypeError: this.model is undefined

Now, I wouldn’t mind, but it’s just the original getter code (I’ve not actually changed it yet!)

what am I missing? Is overriding a getter in this way somehow not supported or does it introduce some kind of scoping issue?

[merefield (Robert)]

Oddly this doesn’t seem to be an issue with Latest. (That was only a test on Production.)

I’ll see if I can reproduce on a clean 3.0.0 Stable.

[david (David Taylor)]

The example should work, and I’m not aware of any relevant changes between the stable release and now. Let us know if you manage to reproduce the issue!

[merefield (Robert)]

thanks David. Will do. Yes I noticed that. Ball is definitely in my court for now as the environment in which I experienced this was not fully clean.

[merefield (Robert)]

OK David, it appears (and this is weird):

• works in Production
• fails in Development 3.0.0 onwards (possibly before)

• I’ve tested on a vanilla no docker Dev environment but also on a very clean (but possibly config incomplete bespoke Docker dev environment).

Here’s the errors:

Error occurred:

- While rendering:
  -top-level
    application
      discourse-root
        sidebar-wrapper
          admin
            admin-wrapper
              nav-item
                link-to
                  -link-to

Uncaught (in promise) TypeError: While generating link to route "adminPlugins": this.model is undefined
    get allAdminRoutes admin-plugins.js:20

note that I can put a debugger statement before the return this.model and it never fires the debugger.

@RGJ has kindly also looked at this and can repro this result.

This is a bit of a problem as clearly we need to develop stuff before it goes to production … but I don’t have to tell you that! Also a bit annoying it’s a problem on Stable …

Moved to Contribute > Bug for now …

[merefield (Robert)]

OK we’re even more sure of there’s an issue here… as mergeMixins is mentioned in the stack trace

https://github.com/emberjs/ember.js/issues/18860

(thanks to @RGJ for finding this)

[david (David Taylor)]

Nice digging - thanks @merefield and @RGJ

I’m not sure how much we can do to fix this within Discourse - as you noted, the issue seems to be upstream in Ember. Perhaps we can patch the problematic method

To get us started, I’ve added a minimal failing test case so we can track progress. There are some comments in the test which describe why the issue is being triggered by our modifyClass system:

https://github.com/discourse/discourse/pull/19911

[merefield (Robert)]

Yeah, thanks for your efforts too. It’s strange they’ve left that issue unaddressed? Do we know if (Ember) 4.x will resolve it?

[david (David Taylor)]

Unfortunately it looks like the issue still exists in Ember 4.x

[merefield (Robert)]

@david

added a minimal failing test case

Funnily enough that seems to be the only progress over at the Ember project, within the associated PR which is still in Draft. I wonder if all that would be required is a polite request to prioritise it when the time becomes available?:

https://github.com/emberjs/ember.js/pull/20129

Fully appreciate this was opened by a staff member at a third party.

Is it worth raising an issue over at GitHub - emberjs/ember.js: Ember.js - A JavaScript framework for creating ambitious web applications · GitHub and referencing this PR, this Topic and your test?

[david (David Taylor)]

Yep, my test is based on theirs (but with the discourse-specific api.modifyClass api). I don’t think we should open a new issue - it’s 100% the same as the one that’s already open. We’ll see if we can do anything to get it prioritised.

[david (David Taylor)]

Making some progress here - we now have a PR with a fix open at [BUGFIX LTS] Don't run getters while applying mixins by wycats · Pull Request #20388 · emberjs/ember.js · GitHub

Once that’s merged and backported to Ember 3.28, we’ll aim to get it applied to Discourse ASAP

[merefield (Robert)]

@david

Once that’s merged and backported to Ember 3.28

Backported?! I wondered about that, awesome news!

Thanks for shepherding this one David!

[david (David Taylor)]

This fix has now been released as part of Ember 3.28.12, and we’ve updated Discourse to use it. Please let us know if you’re still seeing any issues with modifyClass after updating to the latest Discourse.

[merefield (Robert)]

Awesome David, thanks very much!