Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Remote clusters] Convert service files to TypeScript #94138

Merged
merged 3 commits into from
Mar 10, 2021
Merged

[Remote clusters] Convert service files to TypeScript #94138

merged 3 commits into from
Mar 10, 2021

Conversation

yuliacech
Copy link
Contributor

Summary

This PR converts some service files from JavaScript to TypeScript as preparation for more changes needed in Remote Clusters form for Cloud users. For discussion of Cloud work see PR#93953.
The goal is to be able to convert remote_clusters/public/application/sections/components/remote_cluster_form/remote_cluster_form.js to TypeScript while working on Cloud changes.

@yuliacech yuliacech requested a review from a team as a code owner March 9, 2021 16:22
@yuliacech yuliacech added release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.13.0 v8.0.0 labels Mar 9, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@cjcenizal
Copy link
Contributor

@yuliacech Could you please add the chore label to this and similar PRs?

@yuliacech yuliacech added the chore label Mar 9, 2021
alisonelizabeth
alisonelizabeth approved these changes Mar 10, 2021
View reviewed changes
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @yuliacech! It's exciting to see these files getting converted to TS 🎉 . Left one question regarding the use of ! but otherwise code LGTM. I did not run locally as it looks like no functionality has changed.

...rs/public/application/sections/components/remote_cluster_form/validators/validate_address.ts Outdated
@@ -17,14 +17,14 @@ export function isAddressValid(seedNode) {
// no need to wait for regEx if the part is empty
return true;
}
const [match] = part.match(/[A-Za-z0-9\-]*/);
const [match] = part.match(/[A-Za-z0-9\-]*/)!;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we confident that this will never return null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should always get a match because of the * operator (zero or more), but your comment made me consider that the ?? will be more readable, so I updated this line.

...rs/public/application/sections/components/remote_cluster_form/validators/validate_address.ts Outdated
@@ -42,6 +42,6 @@ export function isPortValid(seedNode) {
return false;
}

const isPortNumeric = port.match(/[0-9]*/)[0] === port;
const isPortNumeric = port.match(/[0-9]*/)![0] === port;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@alisonelizabeth
Copy link
Contributor

alisonelizabeth commented Mar 10, 2021 •
edited
Loading

@yuliacech I just noticed CI is failing after approving. I think you will need to remove the old jest snapshots for the validator functions that you migrated and rerun the tests and generate new ones before merging.

@yuliacech
Copy link
Contributor Author

Thank you for the review and thoughtful comments, @alisonelizabeth! I updated the snapshots and waiting for the CI to get green before merging.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
remoteClusters 176.0KB 176.1KB +150.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@yuliacech yuliacech merged commit cce4db8 into elastic:master Mar 10, 2021
@yuliacech yuliacech mentioned this pull request Mar 10, 2021
Merged
yuliacech added a commit that referenced this pull request Mar 10, 2021
@yuliacech
[Remote clusters] Convert service files to TypeScript (#94138) (#94295)
38957c7 
* Converted some js files to ts as preparation for remote clusters work for Cloud

* Updated the snapshots

* Removed non-null assertion operator in favor of a more explicit nullish coalescing operator
@yuliacech yuliacech deleted the remote_clusters_cloud_2 branch April 22, 2021 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alisonelizabeth alisonelizabeth alisonelizabeth approved these changes

Assignees
No one assigned
Labels
chore Feature:CCR and Remote Clusters release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.13.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yuliacech @elasticmachine @cjcenizal @alisonelizabeth @kibanamachine