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

Add missing types AnyIfEmpty, ThemeProps, ThemedStyledProps and StyledProps #4126

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

takurinton
Copy link
Contributor

@takurinton takurinton commented Aug 10, 2023

refs #4062 #4117

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/styled-components/index.d.ts#L37-L43

Add more missing types in addition to #4117.

I added some of them.

  • AnyIfEmpty
  • ThemeProps
  • ThemedStyledProps
  • StyledProps

@takurinton
Copy link
Contributor Author

As @grubersjoe has written, the absence of ThemedStyledProps can be sort of resolved with this method.
#4062 (comment)

I think it's also a strategy to write this in the migration guide.
What do @probablyup think??


type AnyIfEmpty<T extends object> = keyof T extends never ? any : T;

export interface ThemeProps<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could definitely be an alias to something else like ExecutionContext

Copy link
Contributor

@quantizor quantizor Aug 11, 2023

Choose a reason for hiding this comment

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

But it'd need to be on top of #4103 so we can have the first generic param to ExecutionContext be the Theme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it be defined as an alias? Or should I add to the migration guide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@probablyup
#4103 is still WIP, so I felt it wouldn't be too late to merge this PR first.
Until then, how about exporting the conventional ThemeProps once?

What do you think??

Copy link
Contributor

Choose a reason for hiding this comment

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

When I upgraded from v5 to v6, I replaced ThemeProps<T> with ExecutionContext in my codebase.

If we do decide to add this to ease upgrading that it should be aliased instead of declaring a new type. For example:

export interface ThemeProps extends ExecutionContext {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done the same in my codebase, but I'm afraid it won't accept generics.
I think I'll either wait for the ExecutionContext to accept generics in #4103, or temporarily include the code in its current state and include it later in #4103.

@takurinton takurinton changed the title Add missing types AnyIfEmpty, ThemeProps, ThemedStyledProps, StyledProps, and IntrinsicElementsKeys Add missing types AnyIfEmpty, ThemeProps, ThemedStyledProps and StyledProps Aug 11, 2023
Comment on lines 295 to 296
export type ThemedStyledProps<P, T> = P & ThemeProps<T>;
export type StyledProps<P> = ThemedStyledProps<P, AnyIfEmpty<DefaultTheme>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

When I upgraded from v5 to v6, I was able to replace ThemedStyledProps<P, T> with P & ExecutionContext in my codebase.

If we do want to add some aliases for the old v5 types to make upgrading easier, we could map them to the new types like this:

export type ThemedStyledProps<P> = P & ExecutionContext;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that would be fine, but it would lead to inconsistencies in the type arguments of StyledProps.

I think it's better to wait for this to be addressed.
https://github.com/styled-components/styled-components/pull/4103/files#diff-adc775db811d749ad367a5fd61454d85f453f549d088fd97ea1575c3eb3290f4R75-R76

(and, until then, the old-style types should be defined anyway.)

Copy link
Contributor

@bcole808 bcole808 left a comment

Choose a reason for hiding this comment

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

👍 This looks reasonable to me. Aliasing these old type names from v5 to the new equivalent types in v6 should make upgrading easier for folks.

packages/styled-components/src/types.ts Outdated Show resolved Hide resolved
Co-authored-by: Ben Cole <bluemicrobyte@gmail.com>
@@ -299,3 +299,8 @@ export type CSSProp = Interpolation<any>;
export type NoInfer<T> = [T][T extends any ? 0 : never];

export type Substitute<A extends object, B extends object> = FastOmit<A, keyof B> & B;

export interface ThemeProps extends ExecutionContext {}

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is.

As mentioned in #4126 (comment), it needs to be placed on top of #4103. Or merge this first and then follow.
However, in some cases it should be possible to replace it by using ExecutionContext.

@Amerr
Copy link

Amerr commented Feb 20, 2024

Any blocks in this merging PR or is it still in review

@mthorne-godaddy
Copy link

Any update on this?

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.

None yet

6 participants