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

Added TypeScript types and tests for the emotion package. #379

Merged
merged 5 commits into from
Oct 10, 2017

Conversation

cameron-martin
Copy link
Contributor

What:
Added TypeScript types for the emotion package.

Why:
Because users of TypeScript will want these.

How:

  • Added a d.ts file with a "types" field in the package.json pointing to it.
  • Added a .ts file which acts as tests. These are run by just testing that this file type checks using tsc --noEmit.

Checklist:

  • Documentation - N/A.
  • Tests
  • Code complete

Comments:

  • These are just for the emotion package, and not for the react-emotion package. The main thing that is blocking me from implementing the typings for react-emotion is getting the declaration merging working correctly for adding the css prop to all elements. I will send a PR if I get these to work.
  • I've never used lerna before, so I'm not sure if the dependencies are in the correct place (at top level vs in the individual packages).
  • The tests don't fully cover the TypeScript definitions, but some of the functions I have never used so I don't feel comfortable blindly writing tests for things that I don't know what are used for.
  • A PR already exists for this (WIP: Add typescript typings #37), but this seems to be quite outdated.
  • This only tests against the latest version of TypeScript. Potentially the package called test-all-versions could be used to test against multiple versions.
  • Any other issues/comments, just shout.

@cameron-martin
Copy link
Contributor Author

cameron-martin commented Oct 5, 2017

I have added using object literals in the form:

css({
  height: 100,
  width: '100%',
  ':hover': {
    display: 'block'
  }
});

and

const cssObject = {
  height: 100,
  width: '100%',
  ':hover': {
    display: 'block'
  }
};

css`
  font-size: 2rem;
  ${cssObject}
`;

to the definitions.

@cameron-martin
Copy link
Contributor Author

Ah I must also add arrays of CSS objects. Is there anything else that I've missed?

@tkh44
Copy link
Member

tkh44 commented Oct 6, 2017

@cameron-martin The value of the expression can be a function. #381 (comment)

I just want to make sure you had that one as well.

@cameron-martin
Copy link
Contributor Author

cameron-martin commented Oct 6, 2017

Ah yeah thanks. I'm guessing only zero-arity functions? I also missed arrays and calling the css function directly with null as well.

Are the same types valid for interpolations, calling the css function directly with, as values in arrays, and as object values?

@cameron-martin
Copy link
Contributor Author

Also, what is the meaning of true as an object value, as in https://github.com/emotion-js/emotion/blob/master/packages/emotion/test/css.test.js#L45?

@emmatown
Copy link
Member

emmatown commented Oct 6, 2017

@cameron-martin

Are the same types valid for interpolations, calling the css function directly with, as values in arrays, and as object values?

Yep

Also, what is the meaning of true as an object value, as in https://github.com/emotion-js/emotion/blob/master/packages/emotion/test/css.test.js#L45?

That's just a test to make sure that properties with boolean values get removed.

@cameron-martin cameron-martin force-pushed the typescript branch 2 times, most recently from 95615ba to 3f5d93a Compare October 6, 2017 08:20
@cameron-martin
Copy link
Contributor Author

I updated them, hopefully they are correct now.


export function flush(): void;

export function values(cls: string, vars: Interpolation[]): string;
Copy link
Member

Choose a reason for hiding this comment

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

This export doesn't exist anymore, I'm guessing this is from the other TypeScript PR?

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 think so, I removed it.

(): Interpolation;
}

export type CreateStyles<TRet> = ((value: Interpolation) => TRet)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with TypeScript but I think this means that it can only have something as the first argument. createStyles accepts an arbitrary length of arguments, so maybe something like this?

(...values: Interpolation[])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@tkh44
Copy link
Member

tkh44 commented Oct 6, 2017

Ah yeah thanks. I'm guessing only zero-arity functions?

In css calls are zero arity.

In styled calls the call recieves props and context arguments.

@tkh44
Copy link
Member

tkh44 commented Oct 10, 2017

Are you ready to merge this @cameron-martin? I wasn't sure if you wanted to add anything else.

@cameron-martin
Copy link
Contributor Author

Yeah I'm ready. Seems good to me and I'll send types for react-emotion separately :)

@emmatown emmatown merged commit 31e1154 into emotion-js:master Oct 10, 2017
@tkh44
Copy link
Member

tkh44 commented Oct 10, 2017

Thanks!

@tkh44 tkh44 mentioned this pull request Oct 10, 2017
@renatorib renatorib mentioned this pull request Oct 13, 2017
3 tasks
@cameron-martin cameron-martin deleted the typescript branch October 31, 2017 20:59
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