-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Console] Clean up use of any
#95065
[Console] Clean up use of any
#95065
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
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.
Thanks for working on this @jloleysens! It's great to see the removal of any
🎉 ! Code LGTM. I did a smoke test of console and did not notice any regressions.
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.
Latest LGTM. Did a quick smoke test again to confirm everything is still working as expected.
@jloleysens Thanks for tackling this migration! Maybe we'll get a mention in the Kibana contributor newsletter. 😄 Is the fix that you mentioned for an existing bug, or for a bug that the any->unknown migration introduced? If it's for an existing bug, could you describe it more detail with repro steps, change the label for this PR to |
@elasticmachine merge upstream |
@cjcenizal thanks for raising that point!
This is an existing behaviour; not sure when it was introduced though 😅, possibly as part of my previous Console refactors. I could not find any open issues for this and upon closer inspection I am not certain under what conditions this logic will be triggered. I simulated how this bug would manifest by adding a gif to the PR description if this code path actually ran. |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
* cleaned up autocomplete.ts and get_endpoint_from_postition.ts of anys * general clean up of lowering hanging fruit * cleaned up remaining anys, still need to test * fix remaining TS compilation issues * also tidy up use of "as any" and some more ": any" * addressed type issues and introduced the default editor settings object * clean up @ts-ignore * added comments to interface Co-authored-by: Kibana Machine <[email protected]>
* cleaned up autocomplete.ts and get_endpoint_from_postition.ts of anys * general clean up of lowering hanging fruit * cleaned up remaining anys, still need to test * fix remaining TS compilation issues * also tidy up use of "as any" and some more ": any" * addressed type issues and introduced the default editor settings object * clean up @ts-ignore * added comments to interface Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
* cleaned up autocomplete.ts and get_endpoint_from_postition.ts of anys * general clean up of lowering hanging fruit * cleaned up remaining anys, still need to test * fix remaining TS compilation issues * also tidy up use of "as any" and some more ": any" * addressed type issues and introduced the default editor settings object * clean up @ts-ignore * added comments to interface Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
Focus for this PR is to remove as many uses of
any
in the Console plugin as possible. This involved searching the code for instances of: any
andas any
and replacing them withunknown
or proper types where possible.The changes here limit the scope to introducing changes that would be update types only; functionality has been changed as little as possible, but it would be good to do a smoke test of Console locally when reviewing.
There is one fix in the code where range replacement was not using the correct
Position
interface as the other logic branches (see usage ofcolumn: anchorToken.position.column
in diff code below). This would lead to a situation where range replacement would insert duplicated text in front of the "anchor" token - resulting in a nonsense update (see gif). We can be extract to a separate PR, but I think it is nice to include the type fix (and the actual fix) here. This is an edge case it seems. Leaving this behaviour as is since it is outside the scope of this PR to address.Gif of how faulty behaviour would manifest
There are some instances of
any
that remain because they either involved to many functionality related changes or overly complex code:src/plugins/console/public/application/containers/editor/legacy/subscribe_console_resize_checker.ts
src/plugins/console/public/application/models/sense_editor/sense_editor.ts
src/plugins/console/public/lib/es/es.ts
src/plugins/console/server/lib/proxy_config_collection.ts
src/plugins/console/public/application/containers/console_history/console_history.tsx
Not counting use of
any
in test files this leaves ~6 usages of any in the Console plugin.This PR did not address all of the remaining
@ts-ignore
s that still need to be addressed (mostly by migrating JS files to TS).Diff of compiled JS as of current commit