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.

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

Use descriptorForDecorator || descriptorForProperty #1002

Merged

Conversation

AbhinavVishak
Copy link
Contributor

@AbhinavVishak AbhinavVishak commented Aug 1, 2019

Fixes #999.

getDescriptorFor() returns undefined for certain properties ( like computed properties through ember-redux, for example), so wrap it with a logical OR to prevent the _dependentKeys accessor from throwing an exception.

@RobbieTheWagner
Copy link
Member

@pzuraq is this the best fix for this?

@pzuraq
Copy link
Contributor

pzuraq commented Aug 1, 2019

We should replace: https://github.com/emberjs/ember-inspector/blob/master/ember_debug/utils/type-check.js#L33

with:

let { descriptorForDecorator, descriptorForProperty } = Ember.__loader.require('@ember/-internals/metal');

return descriptorForProperty(object, key) || descriptorForDecorator(object[key]);

@RobbieTheWagner
Copy link
Member

@AbhinavVishak do you mind updating the PR to do the changes @pzuraq suggested?

@AbhinavVishak
Copy link
Contributor Author

Sure. Let me try that and see how it behaves.

@AbhinavVishak AbhinavVishak force-pushed the feature/undefined-dependentkeys branch from dc2ceef to b8473d4 Compare August 1, 2019 19:39
@RobbieTheWagner
Copy link
Member

@AbhinavVishak did those changes work well for your use case?

@AbhinavVishak
Copy link
Contributor Author

AbhinavVishak commented Aug 2, 2019

@rwwagner90, sorry I haven't got a chance to test them out yet. Shall I leave a comment when I do so ?
Also, shouldn't the conditionals be flipped to to

descriptorForDecorator(object[key]) || descriptorForProperty(object, key); ?

Check for a normal decorator first, and if not available then check for a property ?

@AbhinavVishak
Copy link
Contributor Author

seems to work as expected. shows the appropriate dependent key for a computed property too. 👍

getDescriptorFor() returns undefined for some objects, so wrap with
a logical OR to prevent the _dependentKeys accessor from throwing an
exception.
@AbhinavVishak AbhinavVishak force-pushed the feature/undefined-dependentkeys branch from b8473d4 to 95aa044 Compare August 2, 2019 15:41
@RobbieTheWagner
Copy link
Member

Also, shouldn't the conditionals be flipped to to

descriptorForDecorator(object[key]) || descriptorForProperty(object, key); ?

@pzuraq does the ordering matter? Should we flip them or leave them as is?

@RobbieTheWagner RobbieTheWagner merged commit 48801cb into emberjs:master Aug 9, 2019
@RobbieTheWagner RobbieTheWagner changed the title Add undefined check for getDescriptorFor() Use descriptorForDecorator || descriptorForProperty Aug 9, 2019
@AbhinavVishak AbhinavVishak deleted the feature/undefined-dependentkeys branch October 15, 2019 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ember inspector fails to inspect certain components
3 participants