Skip to content

feat: add typescript definitions #263

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

Closed
wants to merge 1 commit into from
Closed

Conversation

satya164
Copy link
Member

@satya164 satya164 commented Oct 17, 2018

I've never used typescript, so not sure if it's correct. But would be a good starting point if someone wants to help with typescript definitions.

fixes #197

@satya164 satya164 requested a review from Esemesek October 17, 2018 22:35
@satya164 satya164 force-pushed the @satya164/typescript branch from 4cd1dcf to 33413b8 Compare October 17, 2018 22:35
@callstack-bot
Copy link

callstack-bot commented Oct 17, 2018

Hey @satya164, thank you for your pull request 🤗.
The coverage report for this branch can be viewed here.

index.d.ts Outdated
type StyledTag<T> = (strings: string[], ...exprs: Array<string | number | object | InterpolationFunction<T>>) => StyledComponent<T>;

export const styled: {
<T>(component: T): StyledTag<React.ElementConfig<T>>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Here I want to return different types based on the type of T, i.e. if T is string, StyledTag<{ children?: React.Node, [key: string]: any }>, otherwise StyledTag<React.ElementConfig<T>>

index.d.ts Outdated

export const styled: {
<T>(component: T): StyledTag<React.ElementConfig<T>>,
[tag: string]: StyledTag<{ children?: React.Node, [key: string]: any }>,
Copy link
Member Author

@satya164 satya164 Oct 17, 2018

Choose a reason for hiding this comment

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

Here instead of a generic one, probably will be better if I could do something like:

div: StyledTag<JSX.InstrinsicElements.div>

But instead of having to do it for every tag, could I do it programmatically somehow? Like if this was Flow, I could do:

$ObjMap<JSX.InstrinsicElements, <T>(props: T) => StyledTag<T>>;

index.d.ts Outdated
import * as React from 'react';

declare module "./index" {
export const css: (strings: string[], ...exprs: Array<string | number | object>) => string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Be consistent. Choose between string[] and Array<string>

Copy link
Member Author

Choose a reason for hiding this comment

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

@Esemesek can't man, it's union type

Choose a reason for hiding this comment

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

@satya164 You can probably use (string | number | object)[]

@satya164 satya164 force-pushed the @satya164/typescript branch 8 times, most recently from 6280724 to 91d65db Compare October 18, 2018 19:06
@satya164 satya164 changed the title wip: add typescript definitions feat: add typescript definitions Oct 18, 2018
@satya164 satya164 force-pushed the @satya164/typescript branch 4 times, most recently from f0a7375 to abec70c Compare October 18, 2018 19:14
@satya164 satya164 requested review from thymikee and zamotany October 18, 2018 19:20
@satya164 satya164 force-pushed the @satya164/typescript branch from abec70c to b42fdc6 Compare October 18, 2018 19:21
) => string;

export const cx: (
...classNames: Array<string | false | undefined | null | 0>
Copy link
Member

Choose a reason for hiding this comment

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

Is 0 something special or can be replaced by number?

Copy link
Member Author

Choose a reason for hiding this comment

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

It accepts strings or falsy values. That's why 0 is here, çoz it's falsy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you should extract to separate type to keep in semantically clean.

@@ -0,0 +1,30 @@
/// <reference path="../index.d.ts" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this tripe-slash thing required on top of each test file?

Copy link
Member Author

@satya164 satya164 Oct 18, 2018

Choose a reason for hiding this comment

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

Yeah, otherwise it can't find the module. Not sure if there's a way to avoid it. Do you have a suggestion?

Choose a reason for hiding this comment

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

@satya164 You can include the files on types/tsconfig.json like so:

{
  "include": [
    "index.d.ts",
    "tests/**/*"
  ]
}

Note that you have to put these outside of compilerOptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

@resir014 is this necessary? this comment is outdated. with the updated code I don't need this and typescript is checking the code properly

export const styled: StyledJSXIntrinsics & {
<T>(component: React.ReactType<T>): StyledTag<T>;

[key: string]: StyledTag<{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use Record<string, SomeType> instead of { [key: string]: SomeType }

Copy link
Member Author

Choose a reason for hiding this comment

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

How would I use the record?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is more readable.

) => string;

export const cx: (
...classNames: Array<string | false | undefined | null | 0>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you should extract to separate type to keep in semantically clean.

Copy link
Collaborator

@Esemesek Esemesek left a comment

Choose a reason for hiding this comment

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

🔥

@satya164 satya164 force-pushed the @satya164/typescript branch 2 times, most recently from fb54c93 to f424096 Compare October 18, 2018 20:59
@satya164 satya164 force-pushed the @satya164/typescript branch 2 times, most recently from 7d5c1a0 to e818e69 Compare October 18, 2018 22:13
@satya164 satya164 force-pushed the @satya164/typescript branch from e818e69 to da4a94c Compare October 18, 2018 22:14
@satya164 satya164 force-pushed the @satya164/typescript branch from da4a94c to cc59fb9 Compare October 19, 2018 10:26
@satya164 satya164 force-pushed the @satya164/typescript branch from cc59fb9 to 32bc2f5 Compare October 19, 2018 10:47
Copy link

@resir014 resir014 left a comment

Choose a reason for hiding this comment

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

Overall, this looks good 👍 I left some comments for improvements (and a few others on some outdated diff), but this is good to go.

): string;

function cx(
...classNames: Array<string | false | undefined | null | 0>

Choose a reason for hiding this comment

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

This can probably be extracted into a separate type:

type FalseyValue = false | undefined | null | 0;

...classNames: (string | FalseyValue)[]

Copy link
Member Author

Choose a reason for hiding this comment

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

@resir014 yeah, I did that in a previous commit but then reverted, because it seems that all the type aliases I declare are exported. so the user could do:

import { cx, Falsy} from 'linaria'

Which feels wrong :(

Choose a reason for hiding this comment

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

ah, I see.

@satya164
Copy link
Member Author

Thanks a lot for the review @resir014!

@satya164 satya164 closed this Oct 26, 2018
@satya164 satya164 deleted the @satya164/typescript branch October 26, 2018 11:35
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.

TypeScript support
6 participants