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

[jss] Update TS types #973

Merged
merged 11 commits into from
Feb 21, 2019
Merged

[jss] Update TS types #973

merged 11 commits into from
Feb 21, 2019

Conversation

HenriBeck
Copy link
Member

What would you like to add/fix?
This aligns the types more with @types/jss and adds a few features which were missing before.

Corresponding issue (if exists): #964

@kof
Copy link
Member

kof commented Jan 5, 2019

I can't really review it, lets wait for @appsforartists

@kof kof removed their request for review January 5, 2019 14:05
@appsforartists
Copy link
Member

I'm on vacation until February, and probably won't have much time for reviewing in the mean time.

"This aligns the types more with @types/jss and adds a few features which were missing before."

Can you split this into two PRs - one that imports @types/jss and one that makes whatever changes you are proposing?

@kof
Copy link
Member

kof commented Jan 29, 2019

Lets add changelog and move forward with this, we can always fix things retrospectively since we are in alpha

@appsforartists
Copy link
Member

I'm back from vacation now.

I still don't understand why this isn't just an importing of @types/jss. A bunch of work has gone into that package, and 600k downloads per week have tested it.

Can we please import that package, and then you can make whatever changes you see fit? There's no good reason to start from scratch.

Henri Beck added 4 commits February 12, 2019 04:04
… update-jss-types

* jss-plugin-rule-value-function/fix-process-styles: (35 commits)
  Add formatting TS files
  Update typings
  Use types jss definitions
  Fix TS type definitions (#1030)
  v10.0.0-alpha.10
  Update publish scripts (#1027)
  [docs] Update SSR setup with react-jss (#1018)
  temporarily remove the link to the docs, we need perma links instead #1006
  [jss] Update SheetsManager (#1019)
  warn consumers if themed styles are misused. fix #1005 (#1006)
  Added TypeScript Usage documentation for React JSS (#1009)
  [react-jss] Export the context as __Context from react-jss (#1014)
  [jss-plugin-camel-case] Support css vars better (#1017)
  [docs] Add badges to readmes (#1016)
  Revert "Add tests for css variables in camel case plugins"
  Revert "Fix css variables being hyphenated"
  Revert "Run prettier"
  Run prettier
  Fix css variables being hyphenated
  Add tests for css variables in camel case plugins
  ...

# Conflicts:
#	packages/jss/src/index.d.ts
@HenriBeck
Copy link
Member Author

I have a commit now which replaces the styles, and then I made the required changes on top of that.
They were mostly about incomplete/outdated typings.

@kof
Copy link
Member

kof commented Feb 12, 2019

@HenriBeck did you port all types from the @types/jss package?

@HenriBeck
Copy link
Member Author

Yeah, most of them.

// Sebastian Silbermann <https://github.com/eps1lon>
// Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
// TypeScript Version: 2.3
export type Style = object;
Copy link
Member

Choose a reason for hiding this comment

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

Without importing css from @types/css, you're losing type safety on Style. Can we merge this file too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the file only guaranteed partial type safety on Style and a few things are missing. Also with our plugin API currently, it is really hard to type those things. So, for now, I would like to hold off on typing Style.

There is an open issue about adding types to Style: #806


export type Styles<Name extends string = any> = Record<Name, Style>;
export type Classes<Name extends string = any> = Record<Name, string>;
export type JssValue =
Copy link
Member

Choose a reason for hiding this comment

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

If you merge in css.d.ts, you probably don't need this type alias (as you can use the correct type for each property).

packages/jss/src/index.d.ts Outdated Show resolved Hide resolved
packages/jss/src/index.d.ts Outdated Show resolved Hide resolved
packages/jss/src/index.d.ts Outdated Show resolved Hide resolved
packages/jss/src/index.d.ts Show resolved Hide resolved
packages/jss/src/index.d.ts Outdated Show resolved Hide resolved
@kof
Copy link
Member

kof commented Feb 22, 2019

@HenriBeck no changelog entry?

bhupinderbola pushed a commit to bhupinderbola/jss that referenced this pull request Sep 17, 2019
* Updated jss types

* Fix react-jss types

* Use types jss definitions

* Update typings

* Add formatting TS files

* Fix Style typescript definition

* Add some tests for jss type definitions

* Fix exports
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.

3 participants