avoid boolean coercion in property_set
#15690
Conversation
| let newRoot = getPath(root, newPath); | ||
|
|
||
| if (newRoot) { | ||
| if (newRoot !== null && newRoot !== undefined) { |
There was a problem hiding this comment.
Should this check for typeof newRoot === “function” || (typeof newRoot === ‘object’ && newRoot !== null)?
I don’t think we can call set with other types of objects as the first arg can we?
There was a problem hiding this comment.
those cases are handled by set itself
There was a problem hiding this comment.
also confirms what is in docs :P https://www.emberjs.com/api/ember/2.15/namespaces/Ember/methods/trySet?anchor=trySet
There was a problem hiding this comment.
maybe trySet could be deprecated because it is using set anyway with last parameter as true ?
There was a problem hiding this comment.
or it should be renamed to trySetPath because it only works with path :), trySet(undefined, 'prop', 'val') will still blowup in production currently
There was a problem hiding this comment.
added isDestroyed check too
| let newRoot = getPath(root, newPath); | ||
|
|
||
| if (newRoot) { | ||
| if (newRoot !== null && newRoot !== undefined && !newRoot.isDestroyed) { |
There was a problem hiding this comment.
We shouldn't be hard coding .isDestroyed check here. We moved the flag into meta, we should use that...
There was a problem hiding this comment.
maybe that worth another PR, or I should fix with this one
There was a problem hiding this comment.
bekzod
force-pushed
the
bool-coercion
branch
2 times, most recently
from
98baf5c
to
5f402c4
Compare
|
ping this can be merged I think |
| } else if (!tolerant) { | ||
| throw new EmberError( | ||
| `Property set failed: object in path "${newPath}" could not be found or was destroyed.` | ||
| `Property set failed: object in path "${newPath}" could not be found.` |
There was a problem hiding this comment.
updated message here since destroyed assertion is already in set https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/property_set.ts#L68
|
I think you need to update the "set with path - deprecated" test |
this also makes things consistent with docs https://www.emberjs.com/api/ember/2.15/namespaces/Ember/methods/trySet?anchor=trySet