-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cleanup watch
methodes
#16359
cleanup watch
methodes
#16359
Conversation
@@ -16,8 +16,6 @@ import { | |||
let handleMandatorySetter; | |||
|
|||
export function watchKey(obj, keyName, _meta) { | |||
if (typeof obj !== 'object' || obj === null) { return; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since meta
has assertion for non-object / non-function values
ember.js/packages/ember-metal/lib/meta.js
Line 556 in 76dc4de
assert(`Cannot call \`meta\` on ${typeof obj}`, typeof obj === 'object' || typeof obj === 'function'); |
IMHO, it is better to assert than be silent to uncover unnecessary/invalid calls of
watchKey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the assert check for null objects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
ember.js/packages/ember-metal/lib/meta.js
Line 554 in 76dc4de
assert('Cannot call `meta` on null', obj !== null); |
@@ -79,9 +77,6 @@ if (MANDATORY_SETTER) { | |||
} | |||
|
|||
export function unwatchKey(obj, keyName, _meta) { | |||
if (typeof obj !== 'object' || obj === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since peekMeta
has assertion for non-object / non-function values, it is better to assert than be silent to uncover invalid calls of unwatchKey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, though we should check that this doesn't cause any crashes in a few real world apps before it gets into beta. Can you check that ember observer works correctly with this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checked in both emberobserver and emberaddons, both working properly and no regression in benchmarks https://cl.ly/441V333w3R3R/results.pdf
removes some work from #14571
since then I think root cause might have been fixed