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

[IngestManager] Move RequiredPackage type near DefaultPackages #82188

Merged
merged 3 commits into from
Nov 2, 2020

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented Oct 30, 2020

Summary

  • Move the RequiredPackage type to the same location as DefaultPackages so anyone looking at one will see the other as well
  • New approach to defining & using enum-like values.

The basic outline of the approach

  1. Export a JS object as const from common/constants/* (expose runtime value)
    export const requiredPackages = {
    System: 'system',
    Endpoint: 'endpoint',
    } as const;
  2. Import that JS value and convert to a TS type as type TSType = typeof jsValue
    Screen Shot 2020-11-02 at 2 12 27 PM
  3. The values (right-hand side) of that type can be converted to a union type with ValueOf<X>
    Screen Shot 2020-11-02 at 3 10 22 PM
  4. The values can be accessed as TS types TSType['key'] or JS jsValue.key

This way we

  • still get the readability of TS enums
  • can choose to only import the types of values we need (compile time or runtime)
  • use the TS utility types like Pick, Omit, etc

More detail on 1

  • Value which can be access/iterated at runtime, e.g. Object.values(requiredPackages) or requiredPackages.System
  • narrows the type those exact values, not string
    Screen Shot 2020-11-02 at 2 15 58 PM
  • can't be modified (it's readonly)
    Screen Shot 2020-11-02 at 2 12 09 PM

@jfsiii jfsiii requested a review from a team October 30, 2020 19:30
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Oct 30, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@jfsiii jfsiii requested review from nchaulet and jen-huang October 30, 2020 19:31
@jfsiii jfsiii added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0 labels Oct 30, 2020
@jfsiii jfsiii marked this pull request as draft November 2, 2020 03:50
@jfsiii
Copy link
Contributor Author

jfsiii commented Nov 2, 2020

@elasticmachine merge upstream

@jfsiii jfsiii marked this pull request as ready for review November 2, 2020 20:19
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

page load bundle size

id before after diff
ingestManager 385.5KB 385.8KB +293.0B

History

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

@jfsiii
Copy link
Contributor Author

jfsiii commented Nov 2, 2020

@nchaulet I changed the specific implementation here (object vs array) but merging b/c it's similar to what's approved and will be consistent with forthcoming PR's to replace enums and ValueOf type will be used in those.

@jfsiii jfsiii merged commit f83a47b into elastic:master Nov 2, 2020
jfsiii pushed a commit that referenced this pull request Nov 3, 2020
## Summary

Expands on pattern added in #82188

Replace `enum DataType` with TS type `DataType` & runtime JS `dataTypes`
```diff
- export enum DataType {
-  logs = 'logs',
-  metrics = 'metrics',
-}
+ export const dataTypes = {
+  Logs: 'logs',
+  Metrics: 'metrics',
+ } as const;
+ export type DataType = typeof dataTypes;
```

<img width="513" alt="Screen Shot 2020-11-02 at 5 13 56 PM" src="https://user-images.githubusercontent.com/57655/97926339-f5898c00-1d30-11eb-8935-92aad936aeea.png">
<img width="774" alt="Screen Shot 2020-11-02 at 5 14 05 PM" src="https://user-images.githubusercontent.com/57655/97926341-f5898c00-1d30-11eb-8d68-b39e2904cf85.png">
<img width="780" alt="Screen Shot 2020-11-02 at 5 19 50 PM" src="https://user-images.githubusercontent.com/57655/97926342-f5898c00-1d30-11eb-9799-95d77bfc1757.png">
jfsiii pushed a commit that referenced this pull request Nov 3, 2020
…me object (#82230)

## Summary

Expands on pattern added in #82188

### rutime value is now `outputType.Elasticsearch` (lowercase b/c JS value)

```diff
-  type: OutputType.Elasticsearch,
+  type: outputType.Elasticsearch,
```

<img width="821" alt="Screen Shot 2020-11-02 at 4 46 24 PM" src="https://user-images.githubusercontent.com/57655/97923244-ae4ccc80-1d2b-11eb-9cc9-d98636ca9a40.png">
<img width="594" alt="Screen Shot 2020-11-02 at 4 46 35 PM" src="https://user-images.githubusercontent.com/57655/97923245-aee56300-1d2b-11eb-917e-483a68280e8e.png">
<img width="429" alt="Screen Shot 2020-11-02 at 4 46 47 PM" src="https://user-images.githubusercontent.com/57655/97923246-aee56300-1d2b-11eb-964a-37b496211167.png">

### TS type is `OutputType`
#### Can get union type of values with `ValueOf<OutputType>`
<img width="455" alt="Screen Shot 2020-11-02 at 4 47 31 PM" src="https://user-images.githubusercontent.com/57655/97923247-aee56300-1d2b-11eb-8492-40a9367ad90e.png">

#### Or access as `OutputType['Elasticsearch']`
jfsiii pushed a commit that referenced this pull request Nov 3, 2020
## Summary

Expands on pattern added in #82188

`AgentAssetType` `enum` is only used as part of a single type definition
jfsiii pushed a commit that referenced this pull request Nov 3, 2020
## Summary

Expands on pattern added in #82188

Split `enum AgentPolicyStatus` into `type AgentPolicyStatus` and JS value `agentPolicyStatuses`. Still have type safety

<img width="604" alt="Screen Shot 2020-11-02 at 6 05 29 PM" src="https://user-images.githubusercontent.com/57655/97929572-7ba8d100-1d37-11eb-88db-3785381ebfbf.png">
<img width="931" alt="Screen Shot 2020-11-02 at 6 08 41 PM" src="https://user-images.githubusercontent.com/57655/97929574-7ba8d100-1d37-11eb-8db5-ee5d07d171c6.png">
<img width="605" alt="Screen Shot 2020-11-02 at 6 09 02 PM" src="https://user-images.githubusercontent.com/57655/97929575-7c416780-1d37-11eb-8297-e90ffe695161.png">
jfsiii pushed a commit that referenced this pull request Nov 3, 2020
## Summary

Use new runtime & type values added in #82231 (related to #82188 ) instead of strings. Same results. Stronger type safety.

Some before/after pics below:

<img width="680" alt="Screen Shot 2020-11-03 at 6 40 04 AM" src="https://user-images.githubusercontent.com/57655/97989501-9ff2c500-1dac-11eb-8a2d-96ddadfe2477.png">
<img width="525" alt="Screen Shot 2020-11-03 at 6 40 19 AM" src="https://user-images.githubusercontent.com/57655/97989503-a08b5b80-1dac-11eb-97ee-371a0487fbd3.png">

<img width="790" alt="Screen Shot 2020-11-03 at 7 47 42 AM" src="https://user-images.githubusercontent.com/57655/97989505-a08b5b80-1dac-11eb-9db9-54a2719a85b2.png">
<img width="738" alt="Screen Shot 2020-11-03 at 7 47 34 AM" src="https://user-images.githubusercontent.com/57655/97989504-a08b5b80-1dac-11eb-8d1d-692feafd5fe3.png">


<img width="778" alt="Screen Shot 2020-11-03 at 7 54 17 AM" src="https://user-images.githubusercontent.com/57655/97989507-a123f200-1dac-11eb-879d-ab9be4e060eb.png">
<img width="813" alt="Screen Shot 2020-11-03 at 7 54 45 AM" src="https://user-images.githubusercontent.com/57655/97989508-a123f200-1dac-11eb-8725-c44068d6b233.png">
@jfsiii jfsiii self-assigned this Nov 3, 2020
jfsiii pushed a commit that referenced this pull request Nov 3, 2020
## Summary

Follow pattern from #82188. Same behavior as now. Stronger type safety.

 * `installationSources` is runtime value (JS object)
 * `InstallationSource` is TS type (`typeof installationSources`)
 * `ValueOf<InstallationSource>` is TS union type `'installed' | 'not_installed'`
 * Can access values directly as `installationSources.Installed` (JS) or `InstallationSource['Installed']` (TS)
jfsiii pushed a commit that referenced this pull request Nov 4, 2020
…82512)

## Summary

Follow pattern from #82188. Same behavior as now. Stronger type safety.

 * `defaultPackages` is runtime value (JS object)
 * `DefaultPackage` is TS type (`typeof defaultPackages`)
 * `ValueOf<DefaultPackage>` is TS union type `'system' | 'endpoint'`
 * Can access values directly as `defaultPackages.System` (JS) or `DefaultPackage['System']` (TS)
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 4, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 4, 2020
jfsiii pushed a commit that referenced this pull request Nov 4, 2020
… (#82654)

## Summary

- [x] Move the `RequiredPackage` type to the same location as `DefaultPackages` so anyone looking at one will see the other as well
- [x] New approach to defining & using enum-like values. 

## The basic outline of the approach 
  1. Export a JS object `as const` from `common/constants/*` (expose runtime value)
    https://github.com/elastic/kibana/blob/d66cf601bb0466bd36254f40339ac41f9935436c/x-pack/plugins/ingest_manager/common/constants/epm.ts#L12-L15
  1. Import that JS value and convert to a TS type as `type TSType = typeof jsValue` 
    <img width="484" alt="Screen Shot 2020-11-02 at 2 12 27 PM" src="https://user-images.githubusercontent.com/57655/97910756-2e693700-1d18-11eb-9605-e33ceb64e857.png"> 
  1. The values (right-hand side) of that type can be converted to a union type with `ValueOf<X>`
    <img width="557" alt="Screen Shot 2020-11-02 at 3 10 22 PM" src="https://user-images.githubusercontent.com/57655/97914250-99693c80-1d1d-11eb-9605-92379aa4db38.png">
  1. The values can be accessed as TS types `TSType['key']` or JS  `jsValue.key`

This way we
  - still get the readability of TS `enum`s
  - can choose to only import the types of values we need (compile time or runtime) 
  - use the TS utility types like `Pick`, `Omit`, etc

## More detail on 1
  * Value which can be access/iterated at runtime, e.g. `Object.values(requiredPackages)` or `requiredPackages.System`
  * narrows the type those exact values, not `string`
    <img width="823" alt="Screen Shot 2020-11-02 at 2 15 58 PM" src="https://user-images.githubusercontent.com/57655/97910538-e0ecca00-1d17-11eb-9153-71171e9fba75.png">
  * can't be modified (it's `readonly`) 
      <img width="443" alt="Screen Shot 2020-11-02 at 2 12 09 PM" src="https://user-images.githubusercontent.com/57655/97910753-2dd0a080-1d18-11eb-82d4-3fe635ba3071.png">
jfsiii pushed a commit that referenced this pull request Nov 4, 2020
…82653)

## Summary

Expands on pattern added in #82188

Split `enum AgentPolicyStatus` into `type AgentPolicyStatus` and JS value `agentPolicyStatuses`. Still have type safety

<img width="604" alt="Screen Shot 2020-11-02 at 6 05 29 PM" src="https://user-images.githubusercontent.com/57655/97929572-7ba8d100-1d37-11eb-88db-3785381ebfbf.png">
<img width="931" alt="Screen Shot 2020-11-02 at 6 08 41 PM" src="https://user-images.githubusercontent.com/57655/97929574-7ba8d100-1d37-11eb-8db5-ee5d07d171c6.png">
<img width="605" alt="Screen Shot 2020-11-02 at 6 09 02 PM" src="https://user-images.githubusercontent.com/57655/97929575-7c416780-1d37-11eb-8297-e90ffe695161.png">

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
jfsiii pushed a commit that referenced this pull request Nov 4, 2020
)

## Summary

Expands on pattern added in #82188

Replace `enum DataType` with TS type `DataType` & runtime JS `dataTypes`
```diff
- export enum DataType {
-  logs = 'logs',
-  metrics = 'metrics',
-}
+ export const dataTypes = {
+  Logs: 'logs',
+  Metrics: 'metrics',
+ } as const;
+ export type DataType = typeof dataTypes;
```

<img width="513" alt="Screen Shot 2020-11-02 at 5 13 56 PM" src="https://user-images.githubusercontent.com/57655/97926339-f5898c00-1d30-11eb-8935-92aad936aeea.png">
<img width="774" alt="Screen Shot 2020-11-02 at 5 14 05 PM" src="https://user-images.githubusercontent.com/57655/97926341-f5898c00-1d30-11eb-8d68-b39e2904cf85.png">
<img width="780" alt="Screen Shot 2020-11-02 at 5 19 50 PM" src="https://user-images.githubusercontent.com/57655/97926342-f5898c00-1d30-11eb-9799-95d77bfc1757.png">

# Conflicts:
#	x-pack/plugins/ingest_manager/common/constants/epm.ts
#	x-pack/plugins/ingest_manager/common/types/models/epm.ts
jfsiii pushed a commit that referenced this pull request Nov 5, 2020
…me object (#82230) (#82655)

## Summary

Expands on pattern added in #82188

### rutime value is now `outputType.Elasticsearch` (lowercase b/c JS value)

```diff
-  type: OutputType.Elasticsearch,
+  type: outputType.Elasticsearch,
```

<img width="821" alt="Screen Shot 2020-11-02 at 4 46 24 PM" src="https://user-images.githubusercontent.com/57655/97923244-ae4ccc80-1d2b-11eb-9cc9-d98636ca9a40.png">
<img width="594" alt="Screen Shot 2020-11-02 at 4 46 35 PM" src="https://user-images.githubusercontent.com/57655/97923245-aee56300-1d2b-11eb-917e-483a68280e8e.png">
<img width="429" alt="Screen Shot 2020-11-02 at 4 46 47 PM" src="https://user-images.githubusercontent.com/57655/97923246-aee56300-1d2b-11eb-964a-37b496211167.png">

### TS type is `OutputType`
#### Can get union type of values with `ValueOf<OutputType>`
<img width="455" alt="Screen Shot 2020-11-02 at 4 47 31 PM" src="https://user-images.githubusercontent.com/57655/97923247-aee56300-1d2b-11eb-8492-40a9367ad90e.png">

#### Or access as `OutputType['Elasticsearch']`
jfsiii pushed a commit that referenced this pull request Nov 5, 2020
…2674)

## Summary

Use new runtime & type values added in #82231 (related to #82188 ) instead of strings. Same results. Stronger type safety.

Some before/after pics below:

<img width="680" alt="Screen Shot 2020-11-03 at 6 40 04 AM" src="https://user-images.githubusercontent.com/57655/97989501-9ff2c500-1dac-11eb-8a2d-96ddadfe2477.png">
<img width="525" alt="Screen Shot 2020-11-03 at 6 40 19 AM" src="https://user-images.githubusercontent.com/57655/97989503-a08b5b80-1dac-11eb-97ee-371a0487fbd3.png">

<img width="790" alt="Screen Shot 2020-11-03 at 7 47 42 AM" src="https://user-images.githubusercontent.com/57655/97989505-a08b5b80-1dac-11eb-9db9-54a2719a85b2.png">
<img width="738" alt="Screen Shot 2020-11-03 at 7 47 34 AM" src="https://user-images.githubusercontent.com/57655/97989504-a08b5b80-1dac-11eb-8d1d-692feafd5fe3.png">


<img width="778" alt="Screen Shot 2020-11-03 at 7 54 17 AM" src="https://user-images.githubusercontent.com/57655/97989507-a123f200-1dac-11eb-879d-ab9be4e060eb.png">
<img width="813" alt="Screen Shot 2020-11-03 at 7 54 45 AM" src="https://user-images.githubusercontent.com/57655/97989508-a123f200-1dac-11eb-8725-c44068d6b233.png">
jfsiii pushed a commit that referenced this pull request Nov 5, 2020
… (#82673)

## Summary

Expands on pattern added in #82188

`AgentAssetType` `enum` is only used as part of a single type definition
jfsiii pushed a commit that referenced this pull request Nov 5, 2020
## Summary

Follow pattern from #82188. Same behavior as now. Stronger type safety.

 * `installationSources` is runtime value (JS object)
 * `InstallationSource` is TS type (`typeof installationSources`)
 * `ValueOf<InstallationSource>` is TS union type `'installed' | 'not_installed'`
 * Can access values directly as `installationSources.Installed` (JS) or `InstallationSource['Installed']` (TS)
jfsiii pushed a commit that referenced this pull request Nov 5, 2020
…82512) (#82684)

## Summary

Follow pattern from #82188. Same behavior as now. Stronger type safety.

 * `defaultPackages` is runtime value (JS object)
 * `DefaultPackage` is TS type (`typeof defaultPackages`)
 * `ValueOf<DefaultPackage>` is TS union type `'system' | 'endpoint'`
 * Can access values directly as `defaultPackages.System` (JS) or `DefaultPackage['System']` (TS)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants