eBay / nice-modal-react

A modal state manager for React.
https://ebay.github.io/nice-modal-react
MIT License
2.1k stars 122 forks source link

Fix type of create function for interfaces #129

Closed kabirsky closed 1 year ago

kabirsky commented 1 year ago

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 https://github.com/microsoft/TypeScript/issues/15300#issuecomment-332366024), 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 commented 1 year ago

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 commented 1 year ago

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:

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 commented 1 year ago

Published in v1.2.13. Thanks @kabirsky !