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

fix: replace any in TS defs with generics #79

Merged
merged 2 commits into from
May 27, 2020
Merged

Conversation

web-padawan
Copy link
Member

This should better describe our current API and allow developers to provide their own helpers while keeping type checking support, instead of disabling it completely with any.

@web-padawan web-padawan requested a review from Haprog May 26, 2020 11:56
@Haprog
Copy link
Contributor

Haprog commented May 26, 2020

If I understand this correctly this assumes that the value type of custom field itself is the same as the value type of all of it's children/fields which makes this not so useful.

Imo you should be able to have a e.g. a custom field that contains fields with different value types and also the value type of the custom field itself can differ from those. I'm not sure if there's any practical way to describe that more strictly than using any or unknown.

Maybe we could have a generic class CustomFieldElement<T> so you could at least set the value type for custom field itself (not sure if this is doable with the generator or would we need manually written definition for CustomFieldElement). But I don't know if there's any way to define an ordered list of generic types for the contained fields.

The user can still extend CustomFieldElement and then provide more correct/strict types for their specific case I guess.

We could also say that all the types are string in custom field by default and the user needs to extend and change them if they want to use something else. But I think any might still be a nicer default trade off since it's anyway up to the user how many and what kind of field they put inside custom field. The user can still cast the types to what they actually use if they don't want to extend CustomFieldElement right?

@web-padawan
Copy link
Member Author

If I understand this correctly this assumes that the value type of custom field itself is the same as the value type of all of it's children/fields which makes this not so useful.

This is how it currently works out of the box. That's why I wanted to reflect that in TS defs.

Of course, we could switch to use generics a bit differently:

export type CustomFieldParseValueFn<I, O> = (value: I) => Array<O>;

export type CustomFieldFormatValueFn<I, O> = (inputValues: Array<I>) => O;

export type CustomFieldI18n = {
  parseValue: CustomFieldParseValueFn<string, string>;
  formatValue: CustomFieldFormatValueFn<string, string>;
};

That would still cover the current logic, and should also allow what you requested.

@web-padawan
Copy link
Member Author

Here is a playground link illustrating the alternatives:

  • using generics for input only (this PR)
  • using generics for input and output (above comment)
  • using any (master banch).

@web-padawan
Copy link
Member Author

After some more research, I came to the conclusion that we have to choose in what way our TS definitions would affect developer experience:

  1. using any or unknown allows to use anything, but disables type checking 🤷‍♂️

  2. using generics might help to override i18n when extending CustomFieldElement, but does not allow to provide custom parseValue which allows boolean in addition to string 😞

  3. we want to keep value as a string, so at least that part should be strongly typed 🤓

My current proposal is to change TS definitions like this:

export type CustomFieldParseValueFn = (value: string) => Array<unknown>;

export type CustomFieldFormatValueFn = (inputValues: Array<unknown>) => string;

Here is another playground link to try it out. I will update the PR as well.

Copy link
Contributor

@Haprog Haprog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your examples. I think this is a good compromise.

@types/interfaces.d.ts Show resolved Hide resolved
@web-padawan web-padawan merged commit c53d552 into master May 27, 2020
@web-padawan web-padawan deleted the ts-generics branch May 27, 2020 07:14
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 this pull request may close these issues.

2 participants