-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
Correctly handle array config values and expand flat objects within them. #12662
Correctly handle array config values and expand flat objects within them. #12662
Conversation
@@ -4,7 +4,7 @@ test('flat object', () => { | |||
const obj = { | |||
'foo.a': 1, | |||
'foo.b': 2 | |||
} | |||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added semi-colons where my IDE wasn't happy enough :) But do you think we should re-think where we put them and where we don't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just haven't put my IDE into rant-mode with TypeScript yet ;) We should definitely be consistent with adding them. Hopefully we'll add Prettier soon, which handles it for us
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for Prettier
!
return obj.map((item) => ensureDeepObject(item)); | ||
} | ||
|
||
return Object.keys(obj).reduce((fullObject, propertyKey) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about Array.from(Object.entries())
here instead of Object.keys
, but it requires additional lib for Typescript
(es2017.object
) and not sure we want it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Just add es2017.object
and use Object.entries()
platform/config/ensureDeepObject.ts
Outdated
* Recursively traverses through the object's properties and expands ones with | ||
* dot-separated names into nested objects (eg. { a.b: 'c'} -> { a: { b: 'c' }). | ||
* @param {*} obj Object to traverse through | ||
* @returns {Object} Same object instance with expanded properties. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we should include types in these, or just rely on TypeScript and have e.g.
* @param obj Object to traverse through
* @returns Same object instance with expanded properties.
(The type isn't required in jsdoc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not sure too. In this case it's actually even useless. It's just a habit from the past when I used Google Closure compiler to generate docs (for public API) :) But with Typescript I believe we'll have better tools that will be able to detect all types (hopefully, I'll need to check what is out there).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove the types from jsdoc for now, then look deeper into it later. Should be easy to add back based on the TS types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test additions and fixes. I like it 🎉
* @param obj Object to traverse through. | ||
* @returns Same object instance with expanded properties. | ||
*/ | ||
export function ensureDeepObject(obj: any): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that compiler is complaining about implicit return any
type, so defined it explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, it's because of the map
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
Just merge this when you feel ready :) |
Per
yaml
docs arrays can also include objects, like this:is parsed to: