[BUGFIX beta] Use class inheritance for getters and setters #18314
Conversation
| if (descriptor !== undefined) { | ||
| descriptor.set(obj, keyName, value); | ||
| return value; | ||
| if (!EMBER_METAL_TRACKED_PROPERTIES) { |
There was a problem hiding this comment.
FWIW, I don't think we have a way to test the path where EMBER_METAL_TRACKED_PROPERTIES is disabled (since its on by default now). If we end up having to pull it from 3.13, we'll probably have to manually debug some of these scenarios.
Not really a big deal (we don't expect to pull it), but just wanted to mention it so others are aware...
|
FWIW, I'm not 100% we actually should support this. It seems incredibly odd to me to override a CP and not redeclare the @computed. AFAICT, this is not something we have ever supported (clobbering "removes" in .extend() land). Can you help me understand why this should be allowed? |
|
Exactly, the behavior in I believe this will become much more common as users start trying to convert to tracked properties and native classes. They may have overridden a getter in a subclass, for instance, and made it a CP before, but now want to turn it into a standard native getter that depends on some tracked properties. The fact that the superclass will win here is just a bit counterintuitive I think. |
| if ( | ||
| descriptor !== null && | ||
| typeof descriptor.set === 'function' && | ||
| descriptor.set.name === 'CPSETTER_FUNCTION' |
There was a problem hiding this comment.
Can we actually assume that the function name is preserved? I think we probably need to use some other mechanism for this (e.g. WeakSet) 🤔
There was a problem hiding this comment.
Without debugging the specific failures, I think the IE11 failures here likely indicate that we can't rely on it:
pzuraq
force-pushed
the
bugfix/use-class-inheritance-for-getters-setters
branch
from
2814ad2
to
8296f11
Compare
Currently, `get` and `set` will bypass class inheritance and lookup descriptors for CPs. If they exist, they call them directly. This results in bugs where plain, undecorated getters and setters cannot override CPs on parent classes. This fix converts us to using class inheritance, looking up descriptors recursively on the class to find the actual property that is meant to be accessed for this instance. In the `get` case, this is simple removing the bypass altogether. For the `set` case, this would result in behavioral differences: * An additional `get` triggered just before setting, since `set` gets the previous value in order to know if it should notify. * An additional property change notification for CPs, since they must notify themselves (when set using a native setter, for instance). Instead, this PR looks up the descriptor and checks to see if it's a `CPSETTER` function, and triggers it directly if so.
pzuraq
force-pushed
the
bugfix/use-class-inheritance-for-getters-setters
branch
from
8296f11
to
f046bbe
Compare
|
Not sure what the Tidelift failures are here? That seems new, should I rebase? |
ghost
mentioned this pull request
|
Agreed with everything @rwjblue said about this being a weird situation that should be avoided, but we just chased down a bug in one of our apps came down to a superclass computed winning over a native getter in a subclass 😬 Thanks for getting this squared away! |
Currently,
getandsetwill bypass class inheritance and lookupdescriptors for CPs. If they exist, they call them directly. This
results in bugs where plain, undecorated getters and setters cannot
override CPs on parent classes.
This fix converts us to using class inheritance, looking up descriptors
recursively on the class to find the actual property that is meant to be
accessed for this instance. In the
getcase, this is simple removingthe bypass altogether. For the
setcase, this would result inbehavioral differences:
gettriggered just before setting, sincesetgetsthe previous value in order to know if it should notify.
notify themselves (when set using a native setter, for instance).
Instead, this PR looks up the descriptor and checks to see if it's a
CPSETTERfunction, and triggers it directly if so.As an alternative, we could put all CPSETTER functions directly into a
WeakSetand check to see if the setter function exists in that set, rather than checking itstoString(). I was concerned about perf in doing that, but it would be a bit safer.