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

Adopt TS in theme-ui/color #672

Merged
merged 14 commits into from
Feb 19, 2020
Merged

Adopt TS in theme-ui/color #672

merged 14 commits into from
Feb 19, 2020

Conversation

hasparus
Copy link
Member

@hasparus hasparus commented Feb 15, 2020

Part of #668

I noticed @LekoArts is doing conventional commits and I'm stealing the idea.

Changes

Adopted TypeScript

I commited this in a weird way to preserve file history (two first commits on this branch).

Added jsdoc comments

I've added jsdocs with descriptions from README,
because they're visible in tooltips when you hover a function.
(accessible by ts.getLeadingCommentRanges in Compiler API)

Added grayscale to README

This is a separate commit, so we can revert it easily.
I noticed it's missing and it doesn't look like it's internal API,
so I took to add it.

Possible further work

TSDoc comments parameter descriptions

/**
 * Darken a color by an amount 0–1
 * @param c key from theme.colors
 * @param n number between 0 and 1
 */
export const darken = (c: string, n: number) => (t: Theme) =>
  P.darken(n, g(t, c))

Autocomplete color names dependent on theme type?s correctly.

My colleague has mentioned the fact that we get editor support for
color names in sx prop and functions from @theme-ui/color
during development as a biggest DX limitation of theme-ui.

We can write a typo (like "primry") and it still typechecks

import { ColorMode } from '@theme-ui/core'

// "background" | "text" | "primary" | "secondary" | "muted" | "accent" 
type ColorName = keyof ColorMode;

export const darken = (c: ColorName, n: number) => (t: Theme) =>
  P.darken(n, t.colors[c])

It could be implemented by removing string index signature
from ColorMode and turning ColorMode into an interface so more
properties can be added with declaration merging
and module augmentation.

After such action, instead of string color parameters in theme-ui/color would
be typed as ColorName (or something similar).

I am not sure if it is legal to pass color representation (e.g. "#fff")
to the functions here. I'm looking at g function and I think it isn't, it is
also undocumented, so I'll assume that it's not.

@hasparus hasparus requested a review from mxstbr February 15, 2020 18:45
@hasparus hasparus changed the title Ts color Adopt TS in theme-ui/color Feb 15, 2020
@mxstbr mxstbr mentioned this pull request Feb 15, 2020
32 tasks
@@ -11,6 +13,9 @@
"@theme-ui/css": "^0.3.1",
"polished": "^3.4.1"
},
"peerDependencies": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should come to a consensus here whether to put the dependencies for types in peerDep, devDep or Dep. I opted for devDep: https://github.com/system-ui/theme-ui/pull/671/files#diff-03713682702149e044f8824251410926R22

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, previously the packages depended on common concepts.
These concepts are now described as types and we keep them in @theme-ui/core, so the dependency needs to be noted in package.json.

I think there are two possibilities:

  • @theme-ui/color has @theme-ui/core both in peer dependencies and a dev dependencies.
    • Dev dep only would be a TypeScript build error for users without any previous warning. This is also how I got "semantic error TS2307 Cannot find module '@theme-ui/core'." on Circle CI :D
  • @theme-ui/color depends on @theme-ui/core. It should be okay in monorepo with the same versions. I think I'll go this way since color already depends on css.

After the build there is no import to @theme-ui/core so it won't get bundled for people who use only color utils.

@hasparus
Copy link
Member Author

hasparus commented Feb 16, 2020

I've added ts-jest, mostly because I didn't notice it's not here before changing color/test/index.js to TypeScript 🤦 I can revert it if you'd rather test theme-ui with the same tools it's built with.

ts-jest compiles tests with tsc rather than Babel, so they're typechecked. No need for an additional "typecheck" step in CI. I think this is a good idea for a library, because types in DT were not only typechecked but additionally tested with dtslint's ExpectType and ExpectError.
A small thing to help to ensure that we're not trading off UX for ease of maintenance.

@jxnblk
Copy link
Member

jxnblk commented Feb 17, 2020

Thanks for diving into this! Just a couple quick thoughts:

We can write a typo (like "primry") and it still typechecks

Yes, we want to have better checks for typos from keys you've defined in your theme interface. Not sure what that ultimately will look like, but I'm guessing it's okay to punt on this for now with @theme-ui/color

I am not sure if it is legal to pass color representation (e.g. "#fff")

All sx style objects and these functions should be able to accept either a key from the theme.colors scale or a raw color value. The leniency here is a core part of the API, but we have discussed having an opt-in "strict mode" to allow tighter control over how that works in the future

@hasparus
Copy link
Member Author

Sure, I just wanted to drop some notes.

All sx style objects and these functions should be able to accept either a key from the theme.colors scale or a raw color value. The leniency here is a core part of the API, but we have discussed having an opt-in "strict mode" to allow tighter control over how that works in the future

So string here is the only correct. Thanks.

The dependency on core here could be dropped when @theme-ui/css exports Theme.

@mxstbr
Copy link
Member

mxstbr commented Feb 18, 2020

@hasparus let's use ts-node for all the tests! 💯

Copy link
Member

@mxstbr mxstbr 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 great, thanks for also converting the tests that makes a lot of sense! 🔥

Do you want to move the Theme to css in this PR too so we avoid all weirdness entirely? I think that makes a lot of sense.

@hasparus
Copy link
Member Author

Moving Theme turned out to be a pretty big change. This PR consumed #673.

@hasparus hasparus requested a review from mxstbr February 19, 2020 01:39
Copy link
Collaborator

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

One comment and one request. Looks good!

"esModuleInterop": true,
"moduleResolution": "node"
},
"include": ["src/**/*.ts", "src/**/*.tsx", "src/**/*.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.

Did you run into issues without that explicit include ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, tests get built.

dist
  src
  test
    index.test.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.

Makes sense! Might want to add that to other files in a follow-up PR then

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is a monorepo, we could add tsconfig.json to the root with all base compiler options (like moduleResolution node and no resolveJsonModule) and then use "extends" to inherit them. The tsconfig in root would be useful when you open the entire repo in an IDE.

I'm not sure for 100%, but I think "include" is inherited literally (relative to the new config, not to the parent), so most tsconfigs in packages could look like

{ 
  "extends": "../core/tsconfig.json",
  "compilerOptions": {
    "strict": true // if possible since core is unstrict
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Yes we should totally do this! 👍

packages/custom-properties/package.json Outdated Show resolved Hide resolved
Copy link
Member

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

Amazing! 🔥 Thank you for tackling this!

@mxstbr mxstbr merged commit 325e449 into system-ui:master Feb 19, 2020
@hasparus hasparus deleted the ts-color branch February 19, 2020 21:55
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.

4 participants