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

[APM] Migrate charts to typescript #25514

Closed
wants to merge 1 commit into from
Closed

[APM] Migrate charts to typescript #25514

wants to merge 1 commit into from

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Nov 11, 2018 •
edited by alvarolobato
Loading

No description provided.

@sorenlouv sorenlouv force-pushed the typescript-migration branch from c2257ff to 3ce8392 Compare November 11, 2018 16:21
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

sorenlouv
sorenlouv commented Nov 11, 2018
View reviewed changes
x-pack/plugins/apm/public/store/selectors/chartSelectors.ts
interface Coordinate {
x: number;
y?: number | null;
}
Copy link
Member Author

@sorenlouv sorenlouv Nov 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if Coordinate is the correct term. LMK if you have something more apt.

Copy link
Member

Choose a reason for hiding this comment

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

Seems ok to me

@sorenlouv sorenlouv force-pushed the typescript-migration branch from 3ce8392 to 1239fe4 Compare November 11, 2018 17:30
sorenlouv
sorenlouv commented Nov 11, 2018
View reviewed changes
tsconfig.json Outdated
@@ -1,5 +1,6 @@
{
"compilerOptions": {
"resolveJsonModule": true,
Copy link
Member Author

@sorenlouv sorenlouv Nov 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@mattapperson Following up on #24425 I've added support for importing json files but disabled it for the browser (see tsconfig.browser.json).

Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv sorenlouv force-pushed the typescript-migration branch from 522ef1e to 767af9a Compare November 11, 2018 23:18
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv sorenlouv force-pushed the typescript-migration branch from 767af9a to e2cf2b7 Compare November 13, 2018 00:12
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv sorenlouv mentioned this pull request Nov 13, 2018
Merged
@sorenlouv sorenlouv force-pushed the typescript-migration branch from 9678017 to f0dad8b Compare November 14, 2018 00:30
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sorenlouv sorenlouv force-pushed the typescript-migration branch from f0dad8b to 09706dd Compare November 18, 2018 17:51
@sorenlouv sorenlouv force-pushed the typescript-migration branch from 09706dd to 07801ba Compare November 18, 2018 17:53
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

jasonrhodes
jasonrhodes approved these changes Nov 19, 2018
View reviewed changes
Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

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

Just a couple comments/questions/suggestions but nothing that blocks a merge. Nice work!

x-pack/plugins/apm/public/store/selectors/chartSelectors.ts
@@ -47,11 +62,23 @@ export function getCharts(urlParams, charts) {
};
}

export function getResponseTimeSeries(chartsData) {
interface TimeSerie {
Copy link
Member

Choose a reason for hiding this comment

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

I think TimeSeries would be fine (series is singular and plural) 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I was actually wondering about that. "timeserie" sounded weird. Thanks!

x-pack/plugins/apm/public/store/selectors/chartSelectors.ts
interface Coordinate {
x: number;
y?: number | null;
}
Copy link
Member

Choose a reason for hiding this comment

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

Seems ok to me

x-pack/plugins/apm/typings/lodash.mean.d.ts
@@ -0,0 +1,10 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is because we're using the types from our old lodash version and so we don't have the type for this individual lodash.mean package?

Copy link
Member Author

@sorenlouv sorenlouv Nov 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I tried installing @types/lodash.mean and it's expecting mean to be in lodash (which it's only for 4.x)

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/80b8a358482d159f8feba62dab86c3c75a2bf713/types/lodash.mean/index.d.ts#L9-L10

@sorenlouv sorenlouv mentioned this pull request Nov 19, 2018
Merged
@sorenlouv
Copy link
Member Author

Closing in favor of #25848

@sorenlouv sorenlouv closed this Nov 21, 2018
@sorenlouv sorenlouv deleted the typescript-migration branch November 21, 2018 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattapperson mattapperson mattapperson left review comments

@jasonrhodes jasonrhodes jasonrhodes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sorenlouv @elasticmachine @jasonrhodes @mattapperson