Based on the discussions around #121 and the test we just merged in #662, we are going to convert theme-ui to TypeScript! :tada: The plan is to go package-by-package and gradually move all .js files to .ts files.
As you can see there is a fair amount of packages to convert, so we would love your help! :muscle:
Here is how to convert a package (take a look at #662 for an example):
--tsconfig tsconfig.json option to the "prepare" and "watch" commands in the package.json and copy the tsconfig.json from the core package."types": "dist/index.d.ts" and "source": "src/index.ts" to the package.json.js to .ts and fix all type errors and any types in the generated typedef (dist/index.d.ts). To avoid duplicate work please comment which package you want to work on (as long as nobody else is also working on it) so we can mark it as taken.
Packages that still need to be converted:
These packages will be handled separately (see #950)
Awesome work on this so far! For the preset packages, I wonder if it'd make sense to make a generic theme interface that most of these could follow, or at least use as a base interface
For every package which is typed in DT, we鈥檒l have to clean up and remove it from DT after merge to master. This would be components and theme-ui.
I鈥檓 gonna take color (#672).
I'll add strict: true to its tsconfig if you don't have anything against it.
http://www.typescriptlang.org/docs/handbook/migrating-from-javascript.html#getting-stricter-checks
--strict | boolean | false | Enable all strict type checking options. Enabling --strict enables --noImplicitAny, --noImplicitThis, --alwaysStrict, --strictBindCallApply, --strictNullChecks, --strictFunctionTypes and --strictPropertyInitialization.
-- | -- | -- | --
I'm gonna take:
I'm thinking about taking a stab at typing @theme-ui/css.
Am I right that the css function is wrongly typed in DT?
import { Interpolation, SerializedStyles } from '@emotion/serialize';
import { SystemStyleObject } from '@styled-system/css';
// ...
/**
* A utility from @styled-system/css for theming styles to be passed to Emotion's
* css prop.
*
* Refer:
* 1. https://styled-system.com/css/
* 2. https://emotion.sh/docs/object-styles#with-css
*/
export function css(styleObject: Interpolation): (theme: Theme) => SerializedStyles;
/**
* The `sx` prop accepts a `SxStyleProp` object and properties that are part of
* the `Theme` will be transformed to their corresponding values. Other valid
* CSS properties are also allowed.
*/
export type SxStyleProp = SystemStyleObject;
However the docs say:

So styleObject should be annotated as SystemStyleObject.
I want to work on "preset-base" package.
@hasparus I'm not 100% sure, but I suspect the DT typings might be a little outdated -- I think the typings for this should be completely contained within the theme-ui repo and not rely on Styled System, since that was part of the older implementation. It now uses @theme-ui/css (@styled-system/css will be deprecated in favor of @theme-ui/css at some point)
I'd like to take tailwind and tachyons since they're pretty similar.
How should we handle updating tests to TS?
I鈥檇 like to give components a go if that鈥檚 ok with everybody?
Hey @PaulieScanlon just fyi, there is a package in DT for components. It may be useful to you.
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/theme-ui__components/index.d.ts
@hasparus hey, wow yeah that鈥檚 looks pretty complete! Is it just a case of moving it over and maybe just checking it鈥檚 online with the patterns core has set out?
I want to work on "preset-base" package.
I'd like to take tailwind and tachyons since they're pretty similar.
I鈥檇 like to give components a go if that鈥檚 ok with everybody?
Do it! :100:
How should we handle updating tests to TS?
Once #672 lands we will be using ts-jest to run all our tests, so then we can also rewrite the tests with TypeScript! 馃憤 Follow here: https://github.com/system-ui/theme-ui/pull/672#issuecomment-586704448
Is it just a case of moving it over and maybe just checking it鈥檚 online with the patterns core has set out?
Kinda, but we don't want the index.d.ts file in the repo we want the code itself to be written in TypeScript. I would start by renaming the file from .js to .ts and then using the types in definitelytyped to fill everything out!
Another todo:
.tsconfig.json for all others to extend from as described in by @hasparus https://github.com/system-ui/theme-ui/pull/672#discussion_r381346761 I will do color-modes next!
Please don't convert everything to Typescript!
Love the project, looking to contribute- I'll take a stab at mdx to start if no one has claimed it.
@LA1CH3 do it! 馃憤
I can work on gatsby-plugin-theme-ui as no one has claimed it yet.
I am sorry for my "off-topic" comment, admittedly made as a knee-jerk reaction to coming across this issue. The thing is, I've been using theme-ui since its inception and have also been a fairly active contributor. I care a lot about the project because I really believe in the ideas behind it.
However, I feel like this decision was made without thought to perhaps give time to gain feedback from the community and contributors. While I have no doubt that it's likely most people would favour TypeScript (at least at this point in time), I still think it would have been a reasonable thing to do, as there are valid reasons to stick with JavaScript too! Also, I've been aware of #121 for a while and up until a week ago, it's mostly been discussion around exporting type definitions.
It seems like we are at the point now where it's no longer even a question whether it's reasonable to just drop JavaScript in an existing codebase and swap everything over to TypeScript. To me at least, that doesn't feel right. Anyway that's just my 2c!
@dburles I hear you, and I'm sorry it feels that way. We really love all the contributions you've done so far!
I think this might be due to a bit of private/internal discussion around the matter, and we should have been better about documenting this in the public repo. I'll try to outline some of our thinking with this move:
In the future, we'll try to be better about publicly disclosing bigger decisions like this so that the community can discuss in the open. We dropped the ball here, and I apologize for that.
Hope that helps clarify some of our thinking here, but let me know if you have any questions
I'd like to take gatsby-plugin-theme-ui.
I'd give a shot to gatsby-theme-code-recipes & gatsby-theme-ui-blog
hey I've noticed a minor issue @mxstbr
yarn add @types/something ez.prepare script is ran on local npm install. It's used to build the libraries.@types/something is missing 馃拑I have no better idea than adding following to the package package.json.
"prepare": "npm run build || true",
"test": "tsc --noEmit" // or even "npm run build" just to be sure
@dburles聽I hear you, and I'm sorry it feels that way. We really love all the contributions you've done so far!
@jxnblk Thanks, I enjoy helping out!
Also, thanks for taking the time to outline the thinking behind the refactor. I won't pose questions on the decision as it would be counterproductive. I just hope that it was given adequate thought during the internal discussions and that it ultimately works out for the better.
In the future, we'll try to be better about publicly disclosing bigger decisions like this so that the community can discuss in the open. We dropped the ball here, and I apologize for that.
That would be great, I feel like the project is at the scale where that would make sense. Also, on a semi related note, I鈥檇 love to see a public roadmap to version 1.0 (if one does not already exist).
I didn't claim theme-ui but I kinda added TypeScript there 馃槄
preparescript is ran on local npm install. It's used to build the libraries.
Do I understand you correctly that when any users npm install theme-ui we re-build the package once the files are downloaded? If so, we should for sure not be doing that, right?
preparescript is ran on local npm install. It's used to build the libraries.Do I understand you correctly that when any users
npm install theme-uiwe re-build the package once the files are downloaded? If so, we should for sure not be doing that, right?
I think this doesn't happen.
What I meant by "local npm install" is when a contributor to theme-ui installs packages.
From npm scripts docs.
- prepare: Run both BEFORE the package is packed and published, on local npm install without any arguments, and when installing git dependencies (See below). This is run AFTER prepublish, but BEFORE prepublishOnly.
- install, postinstall: Run AFTER the package is installed.
So prepare runs only for contributors, postinstall is the one you can use for ads.
tsc doesn't exactly fit prepare step, because one can update a package to fix a TypeScript error.\
tsc || true would, but then we'd have to run typecheck in test phase.
Edit: TBH maybe I'm doing something wrong, because reading the "local npm install without any arguments" I understand prepare should not run unless I edit package version manually and then run yarn (not sure if yarn-interactive doesn't also trigger it).
Could use some help here in #752 -- when I try to run yarn test locally, I'm seeing some TS-related errors. I'm not sure if the other PR tests failing are related, but was wondering if anyone could confirm whether or not tests are passing on master. This commit looks like when it started, but I'm not as familiar with tsconfig.json configuration
Hey @jxnblk, making the root tsconfig strict might be a bit too fast right now, let me take a look to make sure.
Yup, oh right. It works like this:
tsc --noEmit to typecheck in all packages would work.packageJson.jest.globals["ts-jest"].tsConfig)I'll loosen up the rules in test overrides.
We could actually disable typechecking in tests at all, and run sth like lerna run typecheck on CI.
@hasparus thanks for looking into this! I'll defer to others with more TS experience, but it seems like typechecking in tests would be helpful -- or at least an easy way to make sure the whole monorepo is working as expected with a single script/command
Here's the PR (https://github.com/system-ui/theme-ui/pull/754)
seems like typechecking in tests would be helpful -- or at least an easy way to make sure the whole monorepo is working as expected with a single script/command
Yup, I totally agree. I don't have any other idea how to respect package tsconfigs than to decouple typechecking from Jest and run it before or after tests. I'll send you a PR to explain what I mean.
BTW did the build get much slower than it used to be?
Heads up, I've just published 0.4.0-alpha.0, available with the @next npm tag. If you'd like to test this out in existing projects, feel free to report any issues you encounter
TODO for color-modes: Use declaration merging on theme-ui/core context value to add colorMode and setColorMode to the type.
https://github.com/DefinitelyTyped/DefinitelyTyped/pull/43135
I will take prism package.
I will also take style-guide package.
As my first Typescript and decent-sized Open Source project contribution; I'm gonna give a try to theme-ui/typography 馃檪
Here are the first commits : https://github.com/system-ui/theme-ui/pull/890
EDIT:
Ok; I'm facing a trouble; maybe someone here can help me :S
(posting here as this is cross-package I believe)
As I needed some libs types (currently unpublished on DT); I decided to:
/types directory using "module augmentation". See this one for example/types diretory inside typeRoots. See hereThis config seems to be working like a charm for the build... but not for the tests !
And indeed; looking at jest configuration, I can see it doesn't even use tsconfig; and is by nature not extendable in child packages (without wizardry).
Furthermore; even using the tsconfig.json wouldn't help; because typesRoots is not even correctly hoisted (see this)
Is there any chance we adopt a pattern that let us write types declarations for libs locally inside packages ? If not; maybe globally for the whole monorepo ?
Yeah, the tests are configured globally.
I've encountered a similar problem https://github.com/system-ui/theme-ui/pull/757.
I have two questions I'll investigate.
/types include in test tsconfig work around this issue?/types directory to define missing library typings when we are building a library? Does it land in user's node_modules?I'll try poking around on your branch to see if I can find out any solution.
Another workaround idea is -- your /types directory is 3 shiny DT PRs waiting to be sent for extra internet points. This may be sweeping the problem under the rug tho.
Edit: BTW I'm getting a syntax error on import type.
(rpt2 plugin) Error: D:/workspace/theme-ui/packages/typography/src/to-theme.ts(6,13): syntax error TS1005 '=' expected.
Error: D:/workspace/theme-ui/packages/typography/src/to-theme.ts(6,13): syntax error TS1005 '=' expected
We should update Babel to 7.9.0 if we're using it.
(and probably add TypeScript 3.8.0 to resolutions?)
Edit: I don't think we can use types directory. Our emitted types will contain imports to untyped libraries and our types dir isn't published. Then, anything we use these types with becomes any. I think we should either move these types to our .ts files and comment that they describe another library or wait until they get merged to DefinitelyTyped. I'd like to be wrong on this.
cc @cyrilchapon
@hasparus thanks for your answer
Wouldn't duplicating /types include in test tsconfig work around this issue? It would be repeated twice, so it's not super not-DRY.
You lost me. Are you talking about using typesRoots in jest config (package.json) ? If so; the pain point is typesRoots config property is not hoisted when extending another tsconfig... (see linked issue in my previous comment). Thus; even placing it on jest config wouldn't help (if one feels like extending the root tsconfig.json). The discussion is actually broader; as any other tsconfig property is currently not being shared between tsconfig and jest-ts-config. I think we could think about an - thus extendable - jest.tsconfig.json instead of hardcoding it in package.json; or even having an external jest.config.js. This would mitigate this. Key points are :
typesRoots, include or any path-related one) ?Can we even use /types directory to define missing library typings when we are building a library? Does it land in user's node_modules?
Well; good catch. Did you test to build and see output to validate that any stuff ? That's a pretty disapointing behavior 馃槩 But still; we can mix the 2 approaches : define the DT-like types in /types (if we allow it internaly); and just export type { Type1, Type2 } from 'the-lib' them. Same result you proposed; but in a structured way. I'm sure of the previous syntax; but maybe even this following could work : export type * from 'the-lib' (untested)
As for export type / import type; good catch. I was wondering. Jest not being that-helpful when it comes to TS errors.
EDIT: and as for DT PRs; I planned to do it... before discovering we have like 30 un-typed themes that each would resolve to a single DT PR 馃槃 See this
I'm sure of the previous syntax; but maybe even this following could work : export type * from 'the-lib' (untested)
I'm afraid we can't reexport them, because the-lib isn't typed in user's computer.
We'd need some rollup magic to actually inline this, so it's probably better to do it manually or wait for DT merge.
馃槩
If so; do we allow ourselves to do same with jest-typescript-config ? (I think we have to, if we allow the first bullet)
The problem we have is that the current test config is really simple -- we run jest once for all packages instead of once per package.
You lost me. Are you talking about using typesRoots in jest config (package.json) ? If so; the pain point is typesRoots config property is not hoisted when extending another tsconfig... (see linked issue in my previous comment). Thus; even placing it on jest config wouldn't help (if one feels like extending the root tsconfig.json).
To work around hoisting we could just write it twice, actually include all external typedefs in jest tsconfig include/typeRoots.
However, I believe this is a non-issue. We can't use it anyway.
Here's a repro https://github.com/hasparus/theme-ui/tree/ts-typography-2.
Notice the type error in https://github.com/hasparus/theme-ui/blob/ts-typography-2/REPRO_PROJECT_USING_TYPOGRAPHY/index.ts#L20.
Ok got it. My PRs will be merged soon I guess, so we should be fine for now
I'll convert try the editor. 馃槉
Just realized I forgot to roll some of the discussion here back into this thread. For this TS conversion, hold off on converting any of the Gatsby plugins or themes because Gatsby consumes these directly (not a compiled version) and there's some advantage to keeping the source code in JS for the shadowing API. We'll need to figure out how to handle that separately in the future when Gatsby supports TS plugins natively. Let me know if anyone has any questions about that
Hej @jxnblk, doesn't https://github.com/gatsbyjs/gatsby/pull/23547 solve this for us?
I didn't check yet, but since we had JSX already in the themes/plugins, it seems like we can just keep uncompiled TypeScript in user's node_modules and Gatsby will compile it. Am I correct?
Can we update this issue description and add:
typographymdx editorprism and done 鉁旓笍 Thanks 馃憤
I'll take:
chrome https://github.com/system-ui/theme-ui/pull/909sidenav https://github.com/system-ui/theme-ui/pull/917Hej @jxnblk, doesn't gatsbyjs/gatsby#23547 solve this for us?
I am not 100% if this is true or not, but either way we wouldn't want to break existing users of the themes and plugins that are using them with older versions of Gatsby, right?
I am not 100% if this is true or not, but either way we wouldn't want to break existing users of the themes and plugins that are using them with older versions of Gatsby, right?
Totally. That would hurt. But if this works, then the PRs currently sent to theme-ui, which add gastsby-plugin-typescript would also work I guess? Although this would probably need a "switch" in gatsby-theme CLI to decide if you want the copied file to be in TypeScript or JS.
We tried two approaches in current open PRs.
prepare step.gatsby-plugin-typescript.I think I prefer the second approach to the first one (which I in used https://github.com/system-ui/theme-ui/pull/705 馃槩). Both shouldn't introduce breaking change. Shadowing a JS file with a TS files works, so I'd assume it also works the other way.
Although Gatsby supports TS natively, I think TS in Gatsby is __"easy to opt-in"__,
not __"opt-out"__, and we should respect the preference of users who dislike TypeScript.
There is a third approach. We could add //@ts-check to these files and write types in JSDoc.
(Similarly to Preact https://github.com/preactjs/preact/blob/master/src/create-element.js#L50)
It has some disadvantages, but we shouldn't need anything more in plugins.
Then, if TypeScript users want the copied/ejected files to be in TypeScript, going from JSDoc comment type annotations to TypeScript isn't hard. At least, it may be less scary than removing type annotations for somebody who doesn't use TypeScript.
@hasparus / @mxstbr Looks like style-guide https://github.com/system-ui/theme-ui/pull/807 is already migrated to ts so we can cross that one too.
@hasparus @mxstbr -- my understanding is that Gatsby's current TS support does NOT work for plugins and themes yet, and as you stated, we do not want to break compatibility with earlier Gatsby versions. I think how we want to handle TS versions of Gatsby plugins is probably worth a bigger discussion, and I think we should move that to a new, separate issue. I'd suggest keeping the in-flight PRs for those packages open, but cut a 0.4 release without those, and addressing those for v0.5. Does that sound reasonable to everyone?
I've opened a new issue to discuss the conversion of the Gatsby-related packages in #950. I've also updated the list in the description here and removed it from the details/summary so that the progress is visible in GitHub's UI.
I'd also suggest we decouple converting the docs site, since that isn't a published package and won't block a v0.4 release once the other packages are ready.
are all done now. @hasparus can you update?
Hey friends!
So, running into some issues. When I import this library, I get a lot of typescript errors right now that prevent me from using it. Specifically errors like this:
node_modules/@theme-ui/components/node_modules/@theme-ui/css/dist/types.d.ts:531:5 - error TS2411: Property 'root' of type 'ThemeUICSSProperties | CSSPseudoSelectorProps | CSSSelectorObject | VariantProperty | UseThemeFunction | undefined' is not assignable to string index type 'ThemeUIStyleObject'.
531 root?: ThemeUIStyleObject;
I cloned down this repo and ran tsc and it resulted in 199 errors. I've noticed that there isn't any CI setup to ensure that types are properly working before merging PRs.
Seems like ya'll need to:
Hey @blainekasten :wave:
I cloned down this repo and ran tsc and it resulted in 199 errors.
All packages in the monorepo have separate TypeScript configs. Many of them are not strict, while root tsconfig is.
We could make root TSConfig less rigid, and override it for stricter options in packages, but I suppose the intention was to recommend strict: true.
Right now to use _theme-ui_ in TypeScript, one has to turn on skipLibChecks to true (this is a default in Next and CRA), and possibly add .d.ts for _@theme-ui/color-modes_ 馃槩
This should be fixed before 0.4.0 release.
Specifically errors like this:
I think this should be fixed in https://github.com/system-ui/theme-ui/pull/1002. I made _@theme-ui/css_ strict there.
Add a CI step to check typescript
This is a good idea.
Right now, the tests and the tested API surface is typechecked in GitHub Actions (ts-jest fails on type errors), but the tsconfig for tests also isn't strict.
I think we should run tsc --noEmit at least on @theme-ui/css and @theme-ui/core to make sure the public API types are correct.
I have a PR open to add typecheck scripts to all packages, though it's grown pretty dusty and has a few merge conflicts.
Knock knock :D Let's regroup.
We have 4 2 packages left.
We can totally leave the .d.ts file in components, as it has few PRs open (https://github.com/system-ui/theme-ui/pull/850, https://github.com/system-ui/theme-ui/pull/823, https://github.com/system-ui/theme-ui/pull/817) and new Variant API will make TS adoption in components easier.
On top of it:
typecheck scripts?@hasparus thanks for the summary! Off the top of my head, I think we'll want to get part of #823 in for the components package, but that needs a closer review. As far as #1002 goes, I can take a look at the docs and leave suggestions if anything seems off
Another thing that I'm not sure whether we need to address for 0.4 or not is upgrading microbundle. It looks like there are some TS/testing related issues in #1022 -- any advise there would be appreciated
hey @jxnblk #1022 is okay with current master (after merging #1002).
However, there is a small problem.
The docs deploy on merge to master, and not on release, so they might be a bit misleading right now. (The TypeScript guide especially.)
@hasparus I've temporarily changed the Netlify settings to not deploy the docs on master, but I can also roll-back to a previous deploy for the docs. I'm not sure how far back to roll them, but can at least find one before #1002 was merged
Hey @mxstbr, some unchecked packages can be checked in the original comment so it's easier to see the progress. Only 2 are left.
any thoughts on porting styled-system to typescript as well? styled-system seems like an important foundational library that theme-ui is built upon
Another thing that I'm not sure whether we need to address for 0.4 or not is upgrading microbundle. It looks like there are some TS/testing related issues in #1022 -- any advise there would be appreciated
@jxnblk we just converted our theme-ui mono-repo based project from microbundle to https://preconstruct.tools which is the same as what emotion uses.
It really sped up our build times (not insignificantly either, from 5 mins down to 12-15s) so it's worth considering. It also simplifies the build in that there only needs to be a single tsconfig and everything passes through babel and let's you use whatever typescript version you want.
Okay, most code in the repo is in TypeScript for a long time already. Let's close this issue.
Hey will there be types definitions (smth like index.d.ts) in this repo in the future?
Currently, if I am not mistaking, you have to also install @types/theme-ui.
I thought closing this issue would imply having all the type definitions in the theme-ui npm package as well.
@MarcStdt Yes, from v0.4.0 on Theme UI provides type definitions for all packages.
@MarcStdt 0.4.0 is not released yet (you can use next tag or 0.4.0-rc.X versions), but the conversion already happened, and the release should happen in the nearest future.
Most helpful comment
@MarcStdt 0.4.0 is not released yet (you can use
nexttag or 0.4.0-rc.X versions), but the conversion already happened, and the release should happen in the nearest future.