@theme-ui/typography => Typescript

[cyrilchapon]

First pass; add typescript, headache with libs, small refactor.

• Base source code transition
• Unit tests transition
• Documentation update

Some notes :

• In the meantime I had to generate declarations for
  • compass-vertical-rhythm
  • modularscale
    (I'll probably PR this to DT; but for now this is in local module augmentations)
• I need advices about TypographyOptions and VerticalRhythm. I had some headache with the various definitions of this across theme-ui, typography, compass-vertical-rhythm. I choose to expose to the world the official typography's TypographyOptions type (through withTheme function, as argument). This is a choice (versus exposing the "custom" - modified - CustomTypographyOptions type the package needs). I'm not sure of this choice. The core idea is
  • Expose official TypographyOptions as argument of the function
  • Clean it, and convert it, right inside the function to make it fit our usage
• I deleted object-assign dependency on the way, and replaced this with plain ES7 ...spread
• Be sure to check my implementation of module augmentation of modularscale and compass-vertical-rhythm; they are special (especially regarding VerticalRhythm type difference across those libs)

I'll probably do unit-tests this evening, along with documentation.

[cyrilchapon]

Doing this; I felt 2 things that I share here :

• I think we need a common BaseTheme type between packages. Related : TypeScript theme definition  theme-specification#8
• typography (the underlying lib) choice could be challengeable. The lib is maintained OK; but the underlying ones (modularscale and compass-vertical-rhythm) don't seem to be that qualitative. The interfaces seem pretty old; type definitions were not ready (which is, often, not a great insight); and the core Javascript is not that readable. There are existing things like shevyjs or polished. What do you think ? Should I file an issue to discuss this ?

[cyrilchapon]

@mxstbr (can't find that "tag as reviewer" feature)

[jxnblk]

FWIW, if it's easier to ditch the dependencies on modularscale and compass-vertical-rhythm and copy relevant code into this package, I think that'd be fine -- I don't anticipate any major changes with those libraries

[cyrilchapon]

@jxnblk yeah why not. Though I hand-wrote some decent type definitions; I'm publishing them to DT this evening and we'll be fine.

The code of modularscale is easily copy / pasteable.
compass-vertical-rhythm is not the same story. It seems pretty complicated and not-that-easily maintainable.

I think my strategy is OK for now; I digged into code; types and created some type "decorators" for internal use. This would introduce some breaking changes; but I definitely think for the mid / long term; we should just rethink the entire stuff with Polished / Shevy. I'd even vote to do this for v1 IMHO; especially as digging into compass-vertical-rhythm was not really a piece of joke.

Suprinsingly, Typography.js on itself seems pretty good (though not-that-typescript ready).

[cyrilchapon]

How to proceed with that snapshot stuff in the tests dir ?

[cyrilchapon]

PRed DefinitelyTyped for :

• modularscale Add modularscale definitions DefinitelyTyped/DefinitelyTyped#44512
• compass-vertical-rhythm Fix compass-vertical-rhythm types, returns and more tests DefinitelyTyped/DefinitelyTyped#44513

Forced typography-theme-xxxx theme types to avoid PRing each of theme (as this is unit-test code)

[hasparus]

BTW I kinda hate that git thinks packages/typography/src/to-theme.ts is new file. The only workaround i know is renaming the file in one commit and changing it in another.

[hasparus]

Aren't you looking for this?

theme-ui/packages/css/src/types.ts

Line 513 in 9388b45

export interface Theme {

@theme-ui/typography is sort of a plugin, so it could probably augment the Theme.

declare module '@theme-ui/css' {
  export interface Theme {
    typography: Typography
  }
}

[cyrilchapon]

Exactly for Theme interface. Giving this a try in the week.

Thanks bud :)

[cyrilchapon]

modularscale PR approved. Waiting for merge

[cyrilchapon]

As seen inside #668; local module augmentation is not really "allowed" in that Monorepo strategy.

To avoid something dirty (copy / paste, local-only typing, ...); I'm just waiting for DT PRs to be aproved + merged + published to finish the work here

[hasparus]

As seen inside #668; local module augmentation is not really "allowed" in that Monorepo strategy.

We can augment stuff in our .ts files. That's how we add sx prop to React. But TBH, waiting for DT is "greater good". Other people can improve the typedefs there, we can use it in other projects etc.

[mrmartineau]

@cyrilchapon has the DT PRs been merged yet? Can we get this PR ready for review again?

[hasparus]

Hey @cyrilchapon, would you be okay with me taking over this PR?
I think it's almost ready to merge. I fixed the conflicts, updated DT deps, and it's looking good.

[hasparus]

Hey, I noticed we're removing bodyColor and few other properties from typography.options.

const unwantedTypographyOptions = [
  'headerColor',
  'bodyColor',
  'overrideStyles',
  'overrideThemeStyles',
  'plugins',
] as const

Just wanted to notice it to make sure it's intentional.
I think bodyColor didn't work when I used typography the last time, but I don't remember it TBH.

[hasparus]

I took the liberty of fixing test fixtures, and merging #1002 here to make @theme-ui/css strict and avoid future merge problems.

[jxnblk]

Just wanted to notice it to make sure it's intentional.

@hasparus yeah, I think the docs mention this, but it's not a 1:1 replacement for how typography.js works, so I don't think any of those options were used before.

[cyrilchapon]

Sorry guys, as we like to say, "life interferred 🙂

@hasparus

Hey @cyrilchapon, would you be okay with me taking over this PR?
I think it's almost ready to merge. I fixed the conflicts, updated DT deps, and it's looking good.

Yep; I'm very glad my branch was usable and a good starting point !

Hey, I noticed we're removing bodyColor and few other properties from typography.options.

const unwantedTypographyOptions = [
  'headerColor',
  'bodyColor',
  'overrideStyles',
  'overrideThemeStyles',
  'plugins',
] as const

@hasparus yeah, I think the docs mention this, but it's not a 1:1 replacement for how typography.js works, so I don't think any of those options were used before.

Yeah I had just made it explicit with this variable, for a core-contributor to review it; because I did not know if it was intentional in the first place. (I literally just typed the existing, not removed / added stuff).

I'm so glad it's merged ❤️

[timonbimon]

Sorry if this is a stupid question, but I am quite new to Typescript 😬
When trying to import @theme-ui/typography the types aren't found (see screenshot). I have "@types/theme-ui": "^0.3.7", in my dependencies. Do you know what I am missing?

[hasparus]

hey @timonbimon, this is actually a good question, and your confusion is understandable.

master branch isn't released as Theme UI 0.3 stable. You'd need to install Theme UI next (0.4) versions.

Theme UI 0.3 is written in JavaScript with types in DefinitelyTyped.
Theme UI 0.4 is written in TypeScript, and you don't need @types/theme-ui anymore.

[timonbimon]

Ok, great, that makes sense (and I see there are already a few release candidates for 0.4)! Thanks a lot for your help. 🤗