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)]

데이비드, 확인해보니 (좀 이상한데):

• 프로덕션에서는 동작함
• 3.0.0 버전부터 개발 환경에서는 실패함 (이전 버전에서도 그랬을 가능성 있음)

• 도커 없이 순수한 개발 환경에서도 테스트해봤고, 매우 깔끔하지만 (아마도 설정이 불완전할 수 있는) 맞춤형 도커 개발 환경에서도 테스트해봤음.

에러 내용은 다음과 같음:

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

return this.model 이전에 디버거 문을 넣어도 디버거가 절대 트리거되지 않는다는 점에 유의해줘.

@RGJ 도 이 문제를 친절하게 살펴봐 주었고, 이 결과를 재현할 수 있다고 함.

이것은 분명히 프로덕션으로 배포하기 전에 개발을 해야 하므로 꽤 큰 문제인데 … 하지만 그건 당신에게 말하지 않아도 알겠지! 또한 스테이블 버전에서도 문제가 발생한다는 점이 좀 짜증남 …

일단 Contribute > Bug 로 옮겼음 …

[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!