[BUGFIX beta] Fix issue with Ember.trySet on destroyed objects.
#13355
Conversation
|
@krisselden / @stefanpenner - r? |
|
Looks ok, but now that if conditional won't be stripped |
|
@stefanpenner - Yep, (a similar |
|
Oops you are correct. This looks good to me |
|
@krisselden - Any objections? |
|
I'm going to assign to @krisselden for review. I want to make sure that we are all happy/ok with this... |
|
I'm ok with this, though, isDestroyed isn't really an ember-metal concept, we should have a value we set meta to, to mean it is destroyed. Maybe then we could fix the long standing issue of recreating meta() on a destroyed object. |
|
@krisselden - Would you prefer that to happen in this PR or can it happen subsequently? |
|
It can be after, I'm sure enforcing not recreating meta will expose some bugs |
|
Please though open a tracking issue, I keep forgetting about. I think our teardown sometimes recreates the mets itself |
| @@ -62,8 +62,11 @@ export function set(obj, keyName, value, tolerant) { | |||
| return setPath(obj, keyName, value, tolerant); | |||
| } | |||
|
|
|||
| assert(`calling set on destroyed object: ${toString(obj)}.${keyName} = ${toString(value)}`, | |||
| !obj.isDestroyed); | |||
| if (obj.isDestroyed) { | |||
There was a problem hiding this comment.
@krisselden should we actually allow setting through destroyed objects or to the descs? My intuition leads me to believe this should actually occur before line 53.
There was a problem hiding this comment.
basically 3 scenarios exist.
- set on plain object property
- set on descriptor
- set on deep object
We currently guard/assert for 1, and I believe we must absolutely also guard/assert for 2. I also believe we should consider the same for 3, if others disagree I would like to be convinced.
|
☔ The latest upstream changes (presumably #13502) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I was wondering about the status of this PR. Looking at this twiddle and checking the console, it seems the issue is still valid on Ember 2.12. |
`Ember.trySet` is designed to be an error-tolerant form of `Ember.set`
(per its public API documentation). However, in some circumstances an
error is still thrown when setting on a destroyed object.
Prior to this change, the following would properly prevent an error:
```js
let obj = { some: { deep : 'path', isDestroyed: true }};
Ember.trySet(obj, 'some.deep', 'stuff');
```
But the following would throw an error incorrectly:
```js
let obj = { some: 'path', isDestroyed: true };
Ember.trySet(obj, 'some', 'stuff');
```
This fixes the latter case...
|
@rwjblue I've rebased this PR onto master. |
Ember.trySetis designed to be an error-tolerant form ofEmber.set(per its public API documentation). However, in some circumstances an error is still thrown when setting on a destroyed object.Prior to this change, the following would properly prevent an error:
But the following would throw an error incorrectly:
This fixes the latter case...
Fixes #12356.