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

ember inspector fails to inspect certain components #999

Closed
AbhinavVishak opened this issue Jul 15, 2019 · 10 comments · Fixed by #1002
Closed

ember inspector fails to inspect certain components #999

AbhinavVishak opened this issue Jul 15, 2019 · 10 comments · Fixed by #1002
Labels

Comments

@AbhinavVishak
Copy link
Contributor

It looks like when the inspector tries to figure out the computed property's dependent keys, it fails.

Ember Inspector has errored.
This is likely a bug in the inspector itself.
You can report bugs at https://github.com/emberjs/ember-inspector.
Error message: Cannot read property '_dependentKeys' of undefined
Stack trace: TypeError: Cannot read property '_dependentKeys' of undefined
    at addProperties (<anonymous>:3159:78)
    at <anonymous>:3120:11
    at Array.forEach (<anonymous>)
    at propertiesForMixin (<anonymous>:3118:20)
    at Class.mixinsForObject (<anonymous>:3008:22)
    at Class.sendObject (<anonymous>:2927:26)
    at Class.inspectById (<anonymous>:2851:16)
    at sendEvent (http://localhost:4200/assets/vendor.js:37100:14)
    at Class.trigger (http://localhost:4200/assets/vendor.js:51786:28)
    at <anonymous>:3502:14

I think it might be related to my use of ember-redux and connect's stateToComputed paradigm.

I tried following through the code and it looks like the descriptorForDecorator call in

return Ember.__loader.require('@ember/-internals/metal').descriptorForDecorator(object[key]);
is the issue. Could somebody clarify if this is something that needs to be fixed on my side, or just not currently supported in the inspector ?

@RobbieTheWagner
Copy link
Member

@AbhinavVishak What version of Ember are you on?

@AbhinavVishak
Copy link
Contributor Author

@rwwagner90 I'm running v3.10.1 on linux

@AbhinavVishak
Copy link
Contributor Author

I'm guessing that since the computed property is being set through an 3rd party addon ember-redux, the DECORATOR_DESCRIPTOR_MAP is not being populated properly, causing the undefined error.

@RobbieTheWagner
Copy link
Member

@AbhinavVishak would it be possible to share your code or create a small reproduction?

@AbhinavVishak
Copy link
Contributor Author

I tried creating a simple hello-world program for ember-redux, and was able to reproduce it.

import Component from '@ember/component';
import { connect } from 'ember-redux';

const stateToComputed = (state) => ({
  computedField: 'hardcoded'
});
const Qwerty = Component.extend({
});
export default connect(stateToComputed)(Qwerty);
<h2>foo</h2>
<h3>{{computedField}}</h3>

@AbhinavVishak
Copy link
Contributor Author

AbhinavVishak commented Jul 16, 2019

from https://github.com/ember-redux/ember-redux/blob/163649b6e2ba9de86d78466f835bed485a76538b/addon/core.js#L85

    Object.keys(props).forEach(key => {
      defineProperty(this, key, computedReduxProperty(key, () => props))
    });

I extracted out the computedReduxProperty value in a breakpoint and tested it against Ember.__loader.require('@ember/-internals/metal').descriptorForDecorator. It was able to find the correct descriptor.

Digging into Ember.defineProperty, it looks like it doesn't set the descriptor directly to the field, but abstracts it through https://github.com/emberjs/ember.js/blob/d212c1926f12ccf7dc9acab024fb7aec76e25c1f/packages/%40ember/-internals/metal/lib/properties.ts#L162

propertyDesc = desc(obj, keyName, undefined, meta);
Object.defineProperty(obj, keyName, propertyDesc);

The inspector searches with propertyDesc , while the map is populated with desc

@RobbieTheWagner
Copy link
Member

@AbhinavVishak thanks for the details! @pzuraq @rwjblue do you guys know if anything around this changed recently or is inspector using the wrong thing in general?

@pzuraq
Copy link
Contributor

pzuraq commented Jul 16, 2019

I think the issue here is that ember-redux is defining computed properties on each instance of the class. When you define a property on the instance, you have to use a different method, descriptorForProperty, to look it up. We can confirm this by trying to define a normal property directly on an instance of the class, and seeing if the inspector blows up.

I think this is something that ember-redux should change, ideally, redefining CPs on every instance is a bad idea. We should also probably create a more general API for looking up the descriptor, if possible.

@AbhinavVishak
Copy link
Contributor Author

AbhinavVishak commented Jul 16, 2019

I was able to trigger this same problem without redux, I think the root cause is Ember.defineProperty, which actually abstracts away the computed property into a regular getter.

import Component from '@ember/component';
import { computed, defineProperty } from '@ember/object';

const MyComp = Component.extend({
  init() {
    this._super(...arguments);
    defineProperty(this, 'computedProp2', computed('dependentProp', function() {
      return this.get('computedProp')+1
    }));
  },

  dependentProp: 1,
  computedProp: computed('dependentProp', function() {
    return this.get('dependentProp')+1
  })
});

export default MyComp;

@pzuraq
Copy link
Contributor

pzuraq commented Jul 16, 2019

Yup, so that confirms it. It works with mixins because mixins still have the computed properties themselves, but doesn't work with the final, defined properties.

What would probably be a good idea would be to crawl the mixins to get the list of properties to lookup, but then only use descriptorForProperty to actually look them up on the instance.

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 a pull request may close this issue.

3 participants