enable templates read a property from a function #15482
Conversation
bekzod
force-pushed
the
show-me-function
branch
3 times, most recently
from
8489fc6
to
10b0ddf
Compare
| @@ -220,7 +220,7 @@ export class NestedPropertyReference extends PropertyReference { | |||
| return parentValue.length; | |||
| } | |||
|
|
|||
| if (typeof parentValue === 'object' && parentValue) { | |||
| if ((typeof parentValue === 'object' || typeof parentValue === 'function') && parentValue) { | |||
There was a problem hiding this comment.
Falsy check should have been !== null and it needs to stay together with the object check. Same with the following. Don't put the function check in between it's confusing it's either an object that isn't null or a function
|
@krisselden according to the original issue, this was working on 2.9. |
bekzod
force-pushed
the
show-me-function
branch
2 times, most recently
from
8975738
to
af58855
Compare
bekzod
changed the title
|
|
||
| this.assertStableRerender(); | ||
|
|
||
| this.runTask(() => set(func, 'aProp', 'still a property on a function')); |
There was a problem hiding this comment.
this is a new feature, the regression was rendering a prop on a function not using Ember.set() to update a prop on a function. https://ember-twiddle.com/c35628b179d00989b7469f84f1d08558?openFiles=controllers.application.js%2C I tried several versions of ember, props on functions were rendered as constant.
There was a problem hiding this comment.
Also, I don't think the watchKey and watchPath is needed for glimmer, just tagForProperty
Since this is a regression, it has to be back ported eventually to LTS, can you please separate out the set(func, 'aProp', ...) updating and the tagForProperty into a separate commit? I don't mind adding that capability, just don't want to do it casually in a bug fix for an LTS regression.
|
|
||
| this.assertStableRerender(); | ||
|
|
||
| this.runTask(() => set(func, 'aProp', 'still a property on a function')); |
There was a problem hiding this comment.
Also, I don't think the watchKey and watchPath is needed for glimmer, just tagForProperty
Since this is a regression, it has to be back ported eventually to LTS, can you please separate out the set(func, 'aProp', ...) updating and the tagForProperty into a separate commit? I don't mind adding that capability, just don't want to do it casually in a bug fix for an LTS regression.
bekzod
force-pushed
the
show-me-function
branch
2 times, most recently
from
49fb515
to
93f9648
Compare
|
I updated this to be focused on just the functionality that regressed. https://github.com/emberjs/ember.js/compare/pr-15482 |
bekzod
changed the title
bekzod
force-pushed
the
show-me-function
branch
2 times, most recently
from
ee67b02
to
e915f22
Compare
|
@bekzod can you resolve the conflict? |
bekzod
force-pushed
the
show-me-function
branch
from
e915f22
to
326dc3d
Compare
bekzod
force-pushed
the
show-me-function
branch
from
326dc3d
to
a1ee4ab
Compare
issue: #14739
broken test: #14729