Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Make "calling set on destroyed object" error more descriptive #12998

Merged
merged 1 commit into from
Feb 23, 2016
Merged

Make "calling set on destroyed object" error more descriptive #12998

merged 1 commit into from
Feb 23, 2016

Conversation

duggiefresh
Copy link
Contributor

Actual code is from PR #12264, I just added a test :)

Closes #10865

@duggiefresh
Copy link
Contributor Author

There's a check here for obj.constructor, because at times obj will be the internal EmptyObject, and I don't know of a better way of checking for it. Otherwise, we receive a "can't turn an object into a primitive" error. ¯_(ツ)_/¯

duggiefresh
duggiefresh reviewed Feb 23, 2016
View reviewed changes
packages/ember-runtime/tests/system/object/observer_test.js
@@ -103,7 +103,7 @@ testBoth('observer should not fire after being destroyed', function(get, set) {

expectAssertion(function() {
set(obj, 'bar', 'BAZ');
}, 'calling set on destroyed object');
}, 'calling set on destroyed object: <(unknown mixin):ember405>.bar = BAZ');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm not 100% sure this is okay to specify the ember id

@duggiefresh duggiefresh force-pushed the 10865-calling-set-on-destroyed-object branch from 3c953e9 to eeffd0e Compare February 23, 2016 04:31
rwjblue
rwjblue reviewed Feb 23, 2016
View reviewed changes
packages/ember-metal/lib/property_set.js
@@ -55,7 +55,8 @@ export function set(obj, keyName, value, tolerant) {
return setPath(obj, keyName, value, tolerant);
}

assert('calling set on destroyed object', !obj.isDestroyed);
assert(`calling set on destroyed object: ${obj.constructor ? obj : null}.${keyName} = ${value}`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does a .toString check satisfy the same edge cases with EmptyObject that this obj.constructor check does? If so, I'd rather use it to make it clearer what we need.

${obj.toString ? obj.toString() : ''} or something?

@rwjblue
Copy link
Member

rwjblue commented Feb 23, 2016

Just had one tiny clarification, but I'm 👍 on this...

@duggiefresh duggiefresh force-pushed the 10865-calling-set-on-destroyed-object branch from eeffd0e to 3a2486e Compare February 23, 2016 15:11
@duggiefresh
Copy link
Contributor Author

Yes that totally works! 🎉 Thank you!

@rwjblue
Copy link
Member

rwjblue commented Feb 23, 2016

LGTM

rwjblue added a commit that referenced this pull request Feb 23, 2016
@rwjblue
Merge pull request #12998 from duggiefresh/10865-calling-set-on-destr…
eac772a 
…oyed-object

Make "calling set on destroyed object" error more descriptive
@rwjblue rwjblue merged commit eac772a into emberjs:master Feb 23, 2016
@rwjblue
Copy link
Member

rwjblue commented Feb 23, 2016

Thanks @duggiefresh!

@homu homu mentioned this pull request Feb 23, 2016
Closed
@bendemboski
Copy link
Contributor

Apologies for my unclear commit comments. Here's what I'm seeing:

This PR introduces a regression where calling set() with an EmptyObject as the value throws a TypeError (on Chrome 48.0.2564.109 and I believe also on the latest Firefox). This is because EmptyObject is not convertible to a string, and the change to the set on destroyed object assertion now unconditionally tries to convert the value to a string to construct the assertion message.

@mmun
Copy link
Member

mmun commented Feb 23, 2016

This PR introduces a regression where calling set() with an EmptyObject...

How is this happening? EmptyObject is not public so it sounds like a framework bug.

@bendemboski
Copy link
Contributor

I am seeing this triggered from Ember Data. An AdapterPopulatedRecordArray is setting a metadata object on itself, but the metadata object is an EmptyObject (https://github.com/emberjs/data/blob/master/addon/-private/system/empty-object.js).

@mmun
Copy link
Member

mmun commented Feb 23, 2016

FYI I have a branch that adds a utils method to handle toStringing in this case, but I haven't merged it in yet...

mmun@116b10b

@bendemboski
Copy link
Contributor

It's this line in Ember Data 2.3.1 -- the store initializes its metadata objects to EmptyObject, so that's meta's value in my repro. That code has since been changed a bit, so it might not occur there with later versions of Ember Data, but I think the underlying issue needs to be addressed in Ember.set regardless.

@mmun
Copy link
Member

mmun commented Feb 23, 2016

@bendemboski Thanks. That usage seems reasonable.

@mmun
Copy link
Member

mmun commented Feb 23, 2016

I'll PR a fix shortly.

@mmun mmun mentioned this pull request Feb 23, 2016
Merged
@duggiefresh duggiefresh deleted the 10865-calling-set-on-destroyed-object branch February 24, 2016 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@duggiefresh @rwjblue @bendemboski @mmun