[EPM] Consume TS types from EPR. Update EPM app code.#54955
[EPM] Consume TS types from EPR. Update EPM app code.#54955jfsiii wants to merge 2 commits intoelastic:feature-ingestfrom jfsiii:updated-registry-types
Conversation
jfsiii
added
the
Feature:EPM
|
Pinging @elastic/ingest (Feature:EPM) |
| "private": true, | ||
| "license": "Elastic-License", | ||
| "dependencies": { | ||
| "@elastic/package-registry": "ssh://git@github.com:jfsiii/package-registry.git#ts-types", |
There was a problem hiding this comment.
Consuming from my PR/fork right now. This can become github.com:elastic/package-registry in the short term, then @elastic/package-registry eventually.
There was a problem hiding this comment.
Should this be https:// instead of ssh:// ? I guess no user permissions are needed.
There was a problem hiding this comment.
Yep, you're right. Updated in 80a5a26.
Will update to elastic/package-registry before merging this.
| className?: string; | ||
| version: RequirementVersion; | ||
| }) { | ||
| export function Version({ className, version }: { className?: string; version?: string }) { |
There was a problem hiding this comment.
The EPR type & golang code have versions as optional
| style={{ width: '22.5rem', maxWidth: '100%' }} | ||
| /> | ||
| {image.src && ( | ||
| <EuiImage |
There was a problem hiding this comment.
The EPR type and golang code have src as optional
| export function createInput(vars: VarsEntry[], inputTemplate: string): string { | ||
| const view: Record<VarsEntry['name'], VarsEntry['default']> = {}; | ||
| export function createInput(vars: Dataset['vars'] = [{}], inputTemplate: string): string { | ||
| const view: Record<string, string> = {}; |
There was a problem hiding this comment.
This one is different from the others.
The existing EPM VarsEntry defines a type with name & default properties. However, the EPR type and golang code only say that it's an object. Essentially a Record<string, any>
There was a problem hiding this comment.
I wonder if there was a reason I made it interface instead of string, meaning for some cases we need it. Suggestion:
- Lets keep it in sync with the registry
- If we change it to
string, lets do a follow up to both repos to make it string, then we also see th effects.
There was a problem hiding this comment.
These are in sync now. That's the change.
EPM had a VarsEntry type which was object with keys of default and name. Both values were strings.
However, EPR has vars as an array of interfaces. Essentially an array of plain objects in JS.
I don't see anything in EPR guaranteeing there will be both default and name in the response or that they must be strings.
EPM probably adds these keys/values somewhere but that makes them an EPM type, not an EPR one.
I could add those in EPM, but I wondered if they were really needed since the type seemed so open. i.e. there could be any string value as a key and anything for the value.
There was a problem hiding this comment.
Lets go forward with this change. 👍
jfsiii
changed the title
nchaulet
left a comment
nchaulet
left a comment
There was a problem hiding this comment.
Really cool how types are generated with struct2ts
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
| export function createInput(vars: VarsEntry[], inputTemplate: string): string { | ||
| const view: Record<VarsEntry['name'], VarsEntry['default']> = {}; | ||
| export function createInput(vars: Dataset['vars'] = [{}], inputTemplate: string): string { | ||
| const view: Record<string, string> = {}; |
There was a problem hiding this comment.
I wonder if there was a reason I made it interface instead of string, meaning for some cases we need it. Suggestion:
- Lets keep it in sync with the registry
- If we change it to
string, lets do a follow up to both repos to make it string, then we also see th effects.
jen-huang
added
the
Team:Fleet
|
I think this would still be nice to have but is not a top priority. I suggest we close it for now but open an issue to track this idea for later. |
|
I agree with @ruflin. I created elastic/package-registry#487 to track one idea I had. |
7 participants
Summary
Use the TS types from elastic/package-registry#196
The app code is largely unchanged but there were a few types which marked properties as optional which were previously typed as required. I'll add notes inline.