Ensure that STATE configuration from static hints are properly configured with multiple levels of class inheritance #288
Conversation
…ured with multiple levels of class inheritance
There was a problem hiding this comment.
Hey @Robert-Frampton, I'm not sure the test is correct, and thus the solution might be incomplete.
Also, while we're at it, the configStateFromStaticHint_ jsdoc says:
/**
* Adds state keys from super classes static hint `MyClass.STATE = {};`.
* @param {Object.<string, !Object>=} opt_config An object that maps all the
* configurations for state keys.
* @protected
*/But I don't see the opt_config used anywhere... can we check to see how was it being used before and update the jsdoc accordingly?
packages/metal-state/src/State.js
Outdated
| State.STATE_REF_KEY = '__METAL_STATE_REF_KEY__'; | ||
|
|
||
| /** | ||
| * Constant used as key on class constructors that extend form State, stores |
There was a problem hiding this comment.
Possible typo form -> from?
| var child = new Child({ | ||
| key2: 'foo2' | ||
| }); | ||
| assert.strictEqual('foo1', test.key1); |
There was a problem hiding this comment.
Shouldn't this be asserting child.key1?
There was a problem hiding this comment.
Actually, if you change it to child.key1, the test will fail...
There was a problem hiding this comment.
No, it's shouldn't be asserting child.key1, key1 isn't being passed a value for Child. I just left this assertion there to ensure that the static hint configuration of Child doesn't affect the values of the parent it's inheriting from.
There was a problem hiding this comment.
Ah, I see! For some reason I assumed they had defaults!
|
Here is the commit in question regarding the inaccurate jsdoc, robframpton@fa26e7c. Looks like it was left over by mistake, that argument was the same as what is now the initialValues_ property. Everything that uses it just accesses the property now rather than passing the argument around. I'll just remove it. |
No description provided.