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 type of create function for interfaces #129

Merged
merged 1 commit into from
Oct 3, 2023
Merged

Conversation

kabirsky
Copy link
Contributor

@kabirsky kabirsky commented Oct 3, 2023

I was hit with interesting type of regression after updating lib from version 1.2.8
After some investigation I found, that interface are not legal extension of Record<string, unknown>, and this is by design (per microsoft/TypeScript#15300 (comment)), since they are potential target of interface merge
Since I don't see any real improvement over P extends {} I thought it would be better to revert this particular change
image
image

@supnate
Copy link
Collaborator

supnate commented Oct 3, 2023

Thanks for report this!
But with {} there's eslint error:

Don't use `{}` as a type. `{}` actually means "any non-nullish value".
- If you want a type meaning "any object", you probably want `Record<string, unknown>` instead.
- If you want a type meaning "any value", you probably want `unknown` instead.
- If you want a type meaning "empty object", you probably want `Record<string, never>` instead.

So how about P extends any ?

@kabirsky
Copy link
Contributor Author

kabirsky commented Oct 3, 2023

Thanks for report this! But with {} there's eslint error:

Don't use `{}` as a type. `{}` actually means "any non-nullish value".
- If you want a type meaning "any object", you probably want `Record<string, unknown>` instead.
- If you want a type meaning "any value", you probably want `unknown` instead.
- If you want a type meaning "empty object", you probably want `Record<string, never>` instead.

So how about P extends any ?

But {} already allowed in .eslintrc:
'@typescript-eslint/ban-types': [ 'error', { extendDefaults: true, types: { '{}': false, }, } ],
Still, let me outline available (and not) variants for this case:

  • <P>, which is an alias for <P extends unknown> - this variant is unavailable since we cant destructure something that might not be an object, and tsc will throw an error
  • <P extends any> has pretty much same problem - since P can be anything it could be not an object, which is incorrect in our case and tsc will throw an error
  • <P extends {}> means 'P is an object', and in older versions of Typescript <P> would be an alias for <P extends {}>, because this type is quite broad, but still removes all primitive and nullish values from our scope
  • <P extends Record<string, unknown>> is as close as we can get to the truth, but since interfaces can be augmented, AND with a non-string key - this type break all interface-oriented code. I am all for using type, but I think we should support all typescript syntax, and in this case it is such an easy task

So, after all considerations, I still think <P extends {}> is the only type that will give us meaning we need that will not unnecessarily break user's code

@supnate supnate merged commit 07471a9 into eBay:main Oct 3, 2023
@supnate
Copy link
Collaborator

supnate commented Oct 3, 2023

Published in v1.2.13. Thanks @kabirsky !

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