merefield
(Robert)
January 14, 2023, 6:57pm
1
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?
4 Likes
merefield
(Robert)
January 16, 2023, 9:45am
2
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.
1 Like
david
(David Taylor)
January 16, 2023, 10:38am
4
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!
1 Like
merefield
(Robert)
January 16, 2023, 10:40am
5
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.
1 Like
merefield
(Robert)
January 16, 2023, 10:23pm
7
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 bug for now …
2 Likes
merefield
(Robert)
January 16, 2023, 10:46pm
8
OK we’re even more sure of there’s an issue here… as mergeMixins is mentioned in the stack trace
opened 06:29PM - 03 Apr 20 UTC
Recently found an issue with mergeMixins. it affects Ember versions at least as … far back as 3.12 and also current release and LTS.
As I understand it, the issue is when a mixin defines a property (doesn’t necessarily have a value) then we try to access the value during construction before owner is set. This breaks getters that access services. Shared findings with @pzuraq.
Error message: `Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container. Error: Assertion Failed: Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container.`
[Code where issue arrises
](https://github.com/emberjs/ember.js/blob/v3.17.3/packages/@ember/-internals/metal/lib/mixin.ts#L475) (Please note: the original link was to `master` 😞. I've changed the tag to the version of ember published the day before, but I have no memory of the original code it linked to)
[Reproduction](https://ember-twiddle.com/3147343e2dbf0e4c6965a50fa7e7389d?openFiles=controllers.application%5C.js%2C) ([Gist](https://gist.github.com/nlfurniss/3147343e2dbf0e4c6965a50fa7e7389d))
(thanks to @RGJ for finding this)
4 Likes
david
(David Taylor)
January 18, 2023, 5:57pm
10
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:
discourse:main
← discourse:modify-class-getters
opened 05:55PM - 18 Jan 23 UTC
https://meta.discourse.org/t/251793/8
https://github.com/emberjs/ember.js/iss… ues/18860
4 Likes
merefield
(Robert)
January 18, 2023, 5:59pm
11
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)
January 18, 2023, 6:05pm
12
Unfortunately it looks like the issue still exists in Ember 4.x
1 Like
merefield
(Robert)
January 18, 2023, 8:53pm
13
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?:
emberjs:master
← emberjs:mixin-merging-18860
opened 05:12PM - 23 Jun 22 UTC
Our handling of getters and setters works correctly for cases where we have only… classic classes, or only native classes, or native classes extending classic classes. In some cases, however, it fails when we have "zebra-striping" of class types, with a classic class extending a native class which extends a (stack of) classic class(es). We correctly handle native getters in mixin and POJOs passed to `.extend()`, but—quite reasonably—do *not* handle getters on native classes when calling `.extend(SomeNativeClass)`. The result is that those getters are not decorated the way they are in mixins, so mixin merging ends up invoking the getter rather than defining a decorator which returns it.
Fixes #18860
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 and referencing this PR, this Topic and your test?
david
(David Taylor)
January 18, 2023, 10:06pm
14
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.
3 Likes
david
(David Taylor)
March 2, 2023, 11:21am
15
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
3 Likes
Backported?! I wondered about that, awesome news!
Thanks for shepherding this one David!
david
(David Taylor)
May 5, 2023, 11:59am
18
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.
6 Likes
Awesome David, thanks very much!
2 Likes
david
(David Taylor)
Closed
May 15, 2023, 7:00am
20
This topic was automatically closed after 9 days. New replies are no longer allowed.