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

Ban inferred types in public interfaces #78126

Closed
mshustov opened this issue Sep 22, 2020 · 2 comments
Closed

Ban inferred types in public interfaces #78126

mshustov opened this issue Sep 22, 2020 · 2 comments
Labels
discuss docs Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@mshustov
Copy link
Contributor

mshustov commented Sep 22, 2020

There are several places in our code base when public interfaces aren't declared explicitly but inferred. That works just fine for the TS compiler, but it's not a human-friendly format.
When working in IDE, it doesn't unfold an inferred type to the final form

The generated documentation suffers from the problem:
2020-09-22_10-47-54

Thus, a developer cannot use public API without digging into the platform (or a plugin) internals.

Ideally, we'd like to declare a linting rule enforcing explicit type declaration for the public types.
That's a recommenced approach used by the api-extractor team. They employ '@typescript-eslint/typedef' rule to enforce type declarations for all memberVariableDeclaration, function params, variable declarations & property declarations
https://github.com/microsoft/rushstack/blob/c4385524b073ca157d29b666e61504d10df80f86/stack/eslint-config/index.js#L483-L492
We could benefit from this approach but it adds a huge overhead for development - only the platform code has 4500+ places to fix when this rule is on! Also the rule explicitly says If you are using stricter TypeScript compiler options, particularly --noImplicitAny and/or --strictPropertyInitialization, you likely don't need this rule. (we do use strict: true). Probably it's not worth the effort since we care about public interfaces only. But there is no easy way to tell whether an interface is public or internal in the context of the linter, since those @public & @internal tags are parsed in the context of TSDoc only.
So we'd have to run a linter against the generated type declarations emitted in the target folder.

The further investigation required, but in the meantime, we could invert the approach for the public interface declaration: declare a public interface explicitly and use it to infer an internal type.

@mshustov mshustov added discuss Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc docs labels Sep 22, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

@pgayvallet
Copy link
Contributor

Moving core to packages did two things:

  • It forced us to get rid of almost all the inferred types in our public types and signatures, so the issue is mostly resolved.

  • It made us learn (the hard way) that getting rid of the remaining 1% wasn't realistically doable and that therefore a hard ban ES rule wasn't an option.

I'm going to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss docs Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

3 participants