objectSpec sub-spec on arrays fix; themeSpec fix #11133
Conversation
Build Artifacts
|
| } | ||
| } | ||
| } | ||
| if (isObject(data) && !isArray(data) && !validateObject(data, options.spec)) { |
There was a problem hiding this comment.
There are some unreachable conditions here, considering the first if ensuring data is only an object or an array. Making this an else if would remove !isArray(data), but also, would isObject and isArray ever be true at the same time?
There was a problem hiding this comment.
It's because isObject is super general and comes back true for an Array (and functions) - so isObject && !isArray needs the !isArray check there because above that bit I'm checking for isArray and handling it when true but don't return so the last step is saying "this is an object that isn't an array" -- which probably should also check "is also not a function".
Although - now that I look at it again, it'd probably be simpler to just return after succeeding within the block where isArray is true and passes validation. Will push an update
There was a problem hiding this comment.
Since this is just for the object spec, perhaps isPlainObject is better? https://lodash.com/docs/4.17.15#isPlainObject
There was a problem hiding this comment.
Bah - nevermind I misread the GH diff - it can't return early.
I added a comment to clarify a bit.
There was a problem hiding this comment.
Awesome thanks for that - all uses of isObject were really trying to ask isPlainObject so I replaced them throughout.
Summary
The
objectSpecutility was not properly validating anArray-type spec's items against the given spec, this ensures that the spec is tested against all child items when given.The
themeSpechad anArray-type property with a default that was not a function as it ought to be, this is fixed.This eliminates the benign but annoying console errors about the above.
Reviewer guidance
Start the server, go to Kolibri, you should not see objectSpec errors in the devtools console.