Optimize EditorConfigProvider merging logic#40250
Conversation
timroes
added
chore
Team:Visualizations
|
Pinging @elastic/kibana-app |
| merged: EditorParamConfig, | ||
| paramName: string | ||
| ): { fixedValue?: any; base?: number } { | ||
| ): { fixedValue: unknown } | { base: number } | {} { |
There was a problem hiding this comment.
ℹ️ We know this method is returning either of those, so we use a union type here instead, which makes more sense.
| merged: EditorParamConfig, | ||
| paramName: string | ||
| ): { timeBase?: string; default?: string } { | ||
| ): { timeBase: string; default: string } { |
There was a problem hiding this comment.
ℹ️ The problem here was, that the method could from it's types return an object without default, that would then legibly fail (in newer TS versions) later on, because TS detects correctly, that a TimeIntervalParam needs this, so this merge method must return it. In fact it also does, if we remove the last if check for isTimeBaseParam(current) which is anyway redundant, since we know that current is a TimeIntervalParam (it's the type of the parameter).
| } | ||
|
|
||
| if (this.isTimeBaseParam(current)) { | ||
| try { |
There was a problem hiding this comment.
ℹ️ I removed the redundant try-catch blocks, that were just rethrowing the caught error.
💚 Build Succeeded |
jen-huang
left a comment
jen-huang
left a comment
There was a problem hiding this comment.
Code LGTM, tested locally on rollup index pattern and hybrid index pattern.
4 participants
Summary
This optimized the
EditorConfigProvidercode a bit (there is no chance in functionality). This will be needed to update to upgrade TypeScript to the next version, because the old could would fail in the newer version (because of some "too broad typings").Review tip: Best to be reviewed with white spaces ignored setting.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] Unit or functional tests were updated or added to match the most common scenarios[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
[ ] This was checked for breaking API changes and was labeled appropriately[ ] This includes a feature addition or change that requires a release note and was labeled appropriately