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

Convert to TypeScript #668

Open
mxstbr opened this issue Feb 14, 2020 · 63 comments
Open

Convert to TypeScript #668

mxstbr opened this issue Feb 14, 2020 · 63 comments

Comments

@mxstbr
Copy link
Member

@mxstbr mxstbr commented Feb 14, 2020

Based on the discussions around #121 and the test we just merged in #662, we are going to convert theme-ui to TypeScript! 🎉 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! 💪

Here is how to convert a package (take a look at #662 for an example):

  1. Add the --tsconfig tsconfig.json option to the "prepare" and "watch" commands in the package.json and copy the tsconfig.json from the core package.
  2. Add "types": "dist/index.d.ts" and "source": "src/index.ts" to the package.json
  3. Go file-by-file, rename them from .js to .ts and fix all type errors and any types in the generated typedef (dist/index.d.ts).
  4. Submit a PR and tag me as a reviewer!

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)

  • gatsby-plugin-theme-ui (#705)
  • gatsby-theme-code-recipes (#709)
  • gatsby-theme-style-guide
  • gatsby-theme-ui-blog (#711)
  • gatsby-theme-ui-layout (#710)
@jxnblk
Copy link
Member

@jxnblk jxnblk commented Feb 14, 2020

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

@hasparus
Copy link
Collaborator

@hasparus hasparus commented Feb 15, 2020

For every package which is typed in DT, we’ll have to clean up and remove it from DT after merge to master. This would be components and theme-ui.

I’m 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.
@LekoArts
Copy link
Contributor

@LekoArts LekoArts commented Feb 15, 2020

I'm gonna take:

  • custom-properties (#671)
  • match-media (#696)
@hasparus
Copy link
Collaborator

@hasparus hasparus commented Feb 16, 2020

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:
image

So styleObject should be annotated as SystemStyleObject.

@Zolwiastyl
Copy link
Contributor

@Zolwiastyl Zolwiastyl commented Feb 16, 2020

I want to work on "preset-base" package.

@jxnblk
Copy link
Member

@jxnblk jxnblk commented Feb 17, 2020

@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)

@joestrouth1
Copy link
Contributor

@joestrouth1 joestrouth1 commented Feb 17, 2020

I'd like to take tailwind and tachyons since they're pretty similar.

How should we handle updating tests to TS?

@PaulieScanlon
Copy link

@PaulieScanlon PaulieScanlon commented Feb 18, 2020

I’d like to give components a go if that’s ok with everybody?

@hasparus
Copy link
Collaborator

@hasparus hasparus commented Feb 18, 2020

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

@PaulieScanlon
Copy link

@PaulieScanlon PaulieScanlon commented Feb 18, 2020

@hasparus hey, wow yeah that’s looks pretty complete! Is it just a case of moving it over and maybe just checking it’s online with the patterns core has set out?

@mxstbr
Copy link
Member Author

@mxstbr mxstbr commented Feb 18, 2020

I want to work on "preset-base" package.
I'd like to take tailwind and tachyons since they're pretty similar.
I’d like to give components a go if that’s ok with everybody?

Do it! 💯

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: #672 (comment)

Is it just a case of moving it over and maybe just checking it’s 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!

@mxstbr
Copy link
Member Author

@mxstbr mxstbr commented Feb 19, 2020

Another todo:

@mxstbr
Copy link
Member Author

@mxstbr mxstbr commented Feb 19, 2020

I will do color-modes next!

@dburles

This comment was marked as off-topic.

@LA1CH3
Copy link
Contributor

@LA1CH3 LA1CH3 commented Feb 20, 2020

Love the project, looking to contribute- I'll take a stab at mdx to start if no one has claimed it.

@mxstbr
Copy link
Member Author

@mxstbr mxstbr commented Feb 21, 2020

@LA1CH3 do it! 👍

@Zolwiastyl
Copy link
Contributor

@Zolwiastyl Zolwiastyl commented Feb 21, 2020

I can work on gatsby-plugin-theme-ui as no one has claimed it yet.

@dburles
Copy link
Contributor

@dburles dburles commented Feb 21, 2020

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!

@hasparus
Copy link
Collaborator

@hasparus hasparus commented May 10, 2020

Hej @jxnblk, doesn't gatsbyjs/gatsby#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?

@ivoreis
Copy link
Contributor

@ivoreis ivoreis commented May 10, 2020

Can we update this issue description and add:

  • #890 to typography
  • #703 next to mdx
  • #900 to editor
  • #805 to prism and done ✔️

Thanks 👍

@ivoreis
Copy link
Contributor

@ivoreis ivoreis commented May 10, 2020

I'll take:

@mxstbr
Copy link
Member Author

@mxstbr mxstbr commented May 11, 2020

Hej @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?

@cyrilchapon cyrilchapon mentioned this issue May 11, 2020
1 of 3 tasks complete
@hasparus
Copy link
Collaborator

@hasparus hasparus commented May 11, 2020

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.

  1. Precompiling in prepare step.
  2. Adding gatsby-plugin-typescript.

I think I prefer the second approach to the first one (which I in used #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.

@ivoreis ivoreis mentioned this issue May 13, 2020
1 of 1 task complete
@ivoreis
Copy link
Contributor

@ivoreis ivoreis commented May 13, 2020

@hasparus / @mxstbr Looks like style-guide #807 is already migrated to ts so we can cross that one too.

@jxnblk
Copy link
Member

@jxnblk jxnblk commented May 19, 2020

@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?

@jxnblk
Copy link
Member

@jxnblk jxnblk commented May 22, 2020

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.

@jxnblk
Copy link
Member

@jxnblk jxnblk commented May 22, 2020

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.

@dcastil
Copy link
Contributor

@dcastil dcastil commented May 22, 2020

are all done now. @hasparus can you update?

@blainekasten
Copy link

@blainekasten blainekasten commented Jun 16, 2020

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:

  1. Add a CI step to check typescript
  2. Fix up all the existing type errors
@hasparus
Copy link
Collaborator

@hasparus hasparus commented Jun 17, 2020

Hey @blainekasten 👋

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 #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.

@hasparus
Copy link
Collaborator

@hasparus hasparus commented Jun 23, 2020

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 (#850, #823, #817) and new Variant API will make TS adoption in components easier.

On top of it:

  • We need this CI step @blainekasten mentioned above.
    • New PR or continue from the one I started with typecheck scripts?
  • I fixed few annoying things in #1002. There's also a draft for TypeScript chapter in the docs there. I'm not a native speaker, so my draft might be actually terrible.
    • Needs another review
  • #907 should be also answered in the docs. This isn't the first time somebody is puzzled about it.
@jxnblk
Copy link
Member

@jxnblk jxnblk commented Jun 26, 2020

@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

@jxnblk
Copy link
Member

@jxnblk jxnblk commented Jun 30, 2020

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

@hasparus
Copy link
Collaborator

@hasparus hasparus commented Jul 1, 2020

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.)

#890 and #1031 are ready for review now.

@jxnblk
Copy link
Member

@jxnblk jxnblk commented Jul 2, 2020

@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

@deadcoder0904
Copy link

@deadcoder0904 deadcoder0904 commented Jul 16, 2020

Hey @mxstbr, some unchecked packages can be checked in the original comment so it's easier to see the progress. Only 2 are left.

@pkieltyka
Copy link

@pkieltyka pkieltyka commented Sep 3, 2020

any thoughts on porting styled-system to typescript as well? styled-system seems like an important foundational library that theme-ui is built upon

@alexanderchan
Copy link

@alexanderchan alexanderchan commented Sep 15, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.