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?
2 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.
2 Likes