Skip to content

fix: type import from csstype #1457

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.

Sign up for GitHub

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

Merged
kof merged 2 commits into from
Feb 25, 2021
Merged

fix: type import from csstype #1457

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf2ce50
fix: type import from csstype
fardad-dev Feb 21, 2021
b692503
fix: wrong type extends in NormalCssValues
fardad-dev Feb 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/jss/src/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import * as css from 'csstype'
import {Properties as CSSProperties} from 'csstype'

// Observable support is included as a plugin. Including it here allows
// TypeScript users to use Observables, which could be confusing if a user
Expand All @@ -10,9 +10,9 @@ import {Observable} from 'indefinite-observable'
// TODO: Type data better, currently typed as any for allowing to override it
type Func<R> = ((data: any) => R)

type NormalCssProperties = css.Properties<string | number>
type NormalCssProperties = CSSProperties<string | number>
type NormalCssValues<K> = K extends keyof NormalCssProperties
? NormalCssProperties[K] | JssValue
? NormalCssProperties[K]
Copy link
Copy Markdown
Member

@moshest moshest Feb 22, 2021

Choose a reason for hiding this comment

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

This seems as a breaking change.. why removing it?

Copy link
Copy Markdown
Member Author

@fardad-dev fardad-dev Feb 22, 2021

Choose a reason for hiding this comment

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

JssValue in there converting NormalCssValues to any.

I think that not needed because when the key is a key of NormalCssProperties we should respond type of that key

Copy link
Copy Markdown
Member

@moshest moshest Feb 22, 2021

Choose a reason for hiding this comment

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

I agree but this can be a breaking change for some users. If that not required I prefer to remove it and maybe push it on a separate PR as breaking change.

Copy link
Copy Markdown
Member Author

@fardad-dev fardad-dev Feb 22, 2021

Choose a reason for hiding this comment

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

i have used jss in huge typescript project, there is nothing broke.

Copy link
Copy Markdown
Member

@moshest moshest Feb 22, 2021

Choose a reason for hiding this comment

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

Still, if someone use a type that's not defined on the standard properties from some reason it will break. No sure if this consider a break or a fix, but I remember some complains in the past about this. NormalCssProperties is not perfect for all properties.

Copy link
Copy Markdown
Member Author

@fardad-dev fardad-dev Feb 23, 2021

Choose a reason for hiding this comment

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

I just checked styled components, they do show an error when an unknown property is used

Screenshot 2021-02-23 at 10 52 16

What about '& .ant-select-selector'?

Copy link
Copy Markdown
Member

@kof kof Feb 23, 2021

Choose a reason for hiding this comment

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

we can probably use for this the new TS feature and identify nesting by "&"

Copy link
Copy Markdown
Member Author

@fardad-dev fardad-dev Feb 23, 2021

Choose a reason for hiding this comment

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

However, you can merge this PR, and we can continue to discuss in an issue.

Copy link
Copy Markdown
Member

@kof kof Feb 23, 2021

Choose a reason for hiding this comment

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

@moshest seems like we can merge it since because of flexible JssValue it should be breaking it right now, but please check it out, I have no code base in TS using it to do so.

Copy link
Copy Markdown
Member

@moshest moshest Feb 23, 2021

Choose a reason for hiding this comment

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

I don't have time to test it right now but I'm ok with merging it. It should be fine for most users anyway..

: JssValue

export type JssStyle =
Expand Down
⚡