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

「DISCUSS」TypeScript utility types #39881

Closed
streamich opened this issue Jun 28, 2019 · 11 comments · Fixed by #41246
Closed

「DISCUSS」TypeScript utility types #39881

streamich opened this issue Jun 28, 2019 · 11 comments · Fixed by #41246
Assignees
Labels

Comments

@streamich
Copy link
Contributor

streamich commented Jun 28, 2019

As we are now actively using TypeScript we accumulate various useful helper utilities, like UnwrapPromise. As I understand we have at least 3 "UnwrapPromise" helpers in our codebase. What shall we do about TypeScript utilities?

  1. Use utility-types.
  2. Use type-fest.
  3. Create our own shared type folder somewhere in /src/types or /src/core.
  4. Do nothing; every plugin can just copy-paste utilities like UnwrapPromise, Omit, Brand, etc.
  5. Create a package like @kbn/types (or @kbn/utility-types) in /packages folder.
@streamich
Copy link
Contributor Author

I would install utility-types.

@streamich streamich changed the title [DISCUSS] TypScript utility types [DISCUSS] TypeScript utility types Jun 28, 2019
@streamich streamich changed the title [DISCUSS] TypeScript utility types 「DISCUSS」TypeScript utility types Jun 28, 2019
@legrego
Copy link
Member

legrego commented Jun 28, 2019

I'm ++ to options 1 and/or 3. I think there's certainly benefit to having these utility types centralized, but I don't know utility-types well enough yet to know if it covers all of our use-cases. We might end up doing a bit of both.

@spalger
Copy link
Contributor

spalger commented Jun 28, 2019

Yeah, I'm a fan of a shared set of utility types that might just re-export types from utility-types and might be custom implementations. But I'd ask that it's a package so I can easily use it from other packages. Happy to set this up if the package bit is a deal breaker.

@streamich
Copy link
Contributor Author

streamich commented Jun 28, 2019

@spalger I added option 5. in OP, are you proposing basically that?

@lukeelmers
Copy link
Member

a shared set of utility types that might just re-export types from utility-types and might be custom implementations

++ this feels like the most flexible option, otherwise we might end up creating our own shared set of utility types later anyway if we need something that lib doesn't offer.

@mshustov
Copy link
Contributor

👍 for 5 to have benefits of 1 & 3.

@joshdover
Copy link
Contributor

(5) sounds great 💯

@timroes
Copy link
Contributor

timroes commented Jul 4, 2019

I vote 5️⃣ with a set of sensible defaults like utility-types (whether or not coming from that library or copied over, I don't mind :D)

@azasypkin
Copy link
Member

Looks like there is a general consensus if favor of option 5. Can we move forward and resurrect (for a slightly different purpose) @kbn/types then? 🙂

@streamich
Copy link
Contributor Author

@azasypkin I would call it @kbn/utility-types because

  1. It will re-export utility-types package
  2. To make it clear that those are utility types
  3. So, we can have the option to use @kbn/types for Kibana types.

@azasypkin
Copy link
Member

  1. To make it clear that those are utility types
  2. So, we can have the option to use @kbn/types for Kibana types.

Sure.

  1. It will re-export utility-types package

It shouldn't matter which package we use under the hood, we can easily switch to another one in the future or write our own utility types from scratch and the name of our own package shouldn't depend on that.

@streamich streamich self-assigned this Jul 16, 2019
@streamich streamich added this to To do in kibana-app-arch via automation Aug 12, 2019
kibana-app-arch automation moved this from To do to Done Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants