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

Generate/publish Typescript types #195

Closed
jfsiii opened this issue Jan 10, 2020 · 7 comments
Closed

Generate/publish Typescript types #195

jfsiii opened this issue Jan 10, 2020 · 7 comments

Comments

@jfsiii
Copy link

jfsiii commented Jan 10, 2020

EPM maintains TS types for the Registry API responses https://github.com/elastic/kibana/blob/0393fea11c57713f6a9523ef463ff03decb912d2/x-pack/legacy/plugins/epm/common/types.ts#L41-L61 but it requires an individual to watch the Registry commits and be sure to update them on the EPM side. This is burdensome and unreliable. See elastic/kibana#52285 which added several properties which were missing entirely and marked others as optional which had been marked as required.

Perhaps EPR could generate TS types as part of a build/release process like it does with the JSON docs from #14

There are a few projects which do this sort of thing. Some I found are

I tried struct2ts and pointed it at the util.Package struct and it produced something quite useful (although there was one issue with the output)

struct2ts -i -m -H util.Package
// this file was automatically generated, DO NOT EDIT
// classes
// struct2ts:github.com/elastic/package-registry/util.PackageRequirementKibana
interface PackageRequirementKibana {
	versions?: string;
}

// struct2ts:github.com/elastic/package-registry/util.PackageRequirement
interface PackageRequirement {
	kibana: PackageRequirementKibana;
}

// struct2ts:github.com/elastic/package-registry/util.PackageImage
interface PackageImage {
	src?: string;
	title?: string;
	size?: string;
	type?: string;
}

// struct2ts:github.com/elastic/package-registry/util.PackageDataSet
interface PackageDataSet {
	title: string;
	name: string;
	release: string;
	type: string;
	ingest_pipeline?: string;
	vars?: map[string]interface {}[];
	package: string;
}

// struct2ts:github.com/elastic/package-registry/util.Package
interface Package {
	name: string;
	title?: string | null;
	version: string;
	readme?: string | null;
	description: string;
	type: string;
	categories: string[];
	requirement: PackageRequirement;
	screenshots?: PackageImage[];
	icons?: PackageImage[];
	assets?: string[];
	internal?: boolean;
	format_version: string;
	datasets?: PackageDataSet[];
	download: string;
	path: string;
}

// exports
export {
	PackageRequirementKibana,
	PackageRequirement,
	PackageImage,
	PackageDataSet,
	Package,
};

There are several options/decisions here (how to convert: use CLI to point at a file/directory or use golang lib to convert a list and write files ourselves? how to share: publish as local to this package or to https://github.com/DefinitelyTyped/DefinitelyTyped, etc) but I think there's a lot of value here for relatively low effort, IMO.

@jfsiii
Copy link
Author

jfsiii commented Jan 10, 2020

Related (at least in spirit) to elastic/kibana#38598

@ruflin
Copy link
Member

ruflin commented Jan 13, 2020

+1 on this. I would suggest to first publish it to this repo and then take it from there. Any suggestion on where in this package we could put these files?

@jfsiii
Copy link
Author

jfsiii commented Jan 13, 2020

Publishing to this repo means adding a package.json file and publishin to npm. Then they can be imported like

import * as EPR from '@elastic/package-registry'; // or whatever it's called

I think types.d.ts or docs/types.d.ts are good candidates, but the location doesn't matter to the consumer. We'd add a types entry to package.json which points to the file. Then you can move it around later and, as long as you update that property, the consumers aren't affected.

As I linked in the description, there's an issue OneOfOne/struct2ts#24 that prevents completely automating the conversion. Actually, I guess we could add sed script to replace that one issue. IIRC, there was a similar issue in another project and their workaround was go add special comments which seemed brittle to me.

We could delay committing to a package name & getting the release process down for now by using yarn link but it requires devs to have the elastic/package-registry repo checked out locally. Not a huge deal (we can add docs to EPM README) but still more work than if it was available on npm.

@jfsiii
Copy link
Author

jfsiii commented Jan 13, 2020

BTW, the value of this was shown again while testing the yarn link approach.

import * as EPR from '@elastic/package-registry';
// ...
export type RegistryPackage = EPR.Package;
Type 'PackageDataSet' is not assignable to type 'Dataset'.
  Types of property 'ingest_pipeline' are incompatible.
    Type 'string | undefined' is not assignable to type 'string'.
      Type 'undefined' is not assignable to type 'string'.

Sure enough, the manually maintained Dataset type has ingest_pipeline as required (ingest_pipeline: string;)

While the util/dataset.go has it marked as optional (omitempty)

IngestPipeline string `config:"ingest_pipeline,omitempty" config:"ingest_pipeline" json:"ingest_pipeline,omitempty"`

The output generated by struct2ts got it right (ingest_pipeline?: string;)

@jfsiii
Copy link
Author

jfsiii commented Jan 13, 2020

@ruflin I can submit a PR with the required items I mentioned in #195 (comment) if you like.

The only thing I'd need to know is if you have an opinion on how to generate the types. e.g. should it be another side effect of go test . -generate or is there a more appropriate command?

@ruflin
Copy link
Member

ruflin commented Jan 14, 2020

I would make it part of mage build: https://github.com/elastic/package-registry/blob/master/magefile.go#L39 The Build command is also called when running mage check, so if it gets outdated, CI will tell us.

Would be great if you can open a PR with it.

@ruflin
Copy link
Member

ruflin commented Jun 22, 2020

Closing this issue as I think the OpenAPI approach replaces this. @jfsiii Please reopen if you think otherwise.

@ruflin ruflin closed this as completed Jun 22, 2020
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 a pull request may close this issue.

2 participants