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.

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

[APM] Migrate to Typescript and refactor backend apis #25848

Merged
merged 9 commits into from
Nov 22, 2018

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Nov 18, 2018

Extends the work done in #25514

Prefer unknown over any

TypeScript 3.0 introduces a new top type unknown. unknown is the type-safe counterpart of any. Anything is assignable to unknown, but unknown isn’t assignable to anything but itself and any without a type assertion or a control flow based narrowing. Likewise, no operations are permitted on an unknown without first asserting or narrowing to a more specific type.
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-0.html

Force explicit return type when using "blackbox" clients
"Blackbox clients" is a term I used about clients where the return type cannot possibly be inferred in compile time. This is the case with the http client, ES client and config manager, that all currently have any as return type. This means that the user is able to access properties and methods that might or might not exists, without any warnings. Example with the ES client:

const response = await client('search', esParams);
response.foo.bar.baz; // no error or warning. The return type for `baz` is also `any`

To highlight this issue I've made all clients generic, so they accept a return type. The default return type is void:

const response = await client<Transaction>('search', esParams);
response.foo.bar.baz; // Error, not available on Transaction
resp.hits.hits[0]._source.transaction.id // Works, and `id` is typed according to the `Transaction` interface

Types for chart
Both the backend and frontend code the charts was in js and has been migrated to ts

@sorenlouv sorenlouv force-pushed the refactor-backend-apis branch from 1426102 to 9dbf5e4 Compare November 18, 2018 20:24
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv sorenlouv force-pushed the refactor-backend-apis branch from 9dbf5e4 to e0d48a2 Compare November 18, 2018 23:31
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv sorenlouv force-pushed the refactor-backend-apis branch 4 times, most recently from c7e57e8 to 65ec5b4 Compare November 19, 2018 01:26
@elastic elastic deleted a comment from elasticmachine Nov 19, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sorenlouv
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jasonrhodes jasonrhodes self-requested a review November 20, 2018 12:55
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.

  • For the API response types, do you want to stick with the I prefix?
  • I like the fetch/transform abstractions, on the fence about whether to pull fetch stuff into the index files or not. If we keep them separate, can we either have fetch/transform or fetcher/transformer for the file names? (I think I prefer fetch/transform but either way.)

Otherwise I think this LGTM

x-pack/plugins/apm/public/utils/url.tsx Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv sorenlouv force-pushed the refactor-backend-apis branch from bacaae4 to 2884ebe Compare November 21, 2018 23:12
@elasticmachine
Copy link
Contributor

💔 Build Failed

@sorenlouv
Copy link
Member Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants