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

Add TypeScript tooling and configs #2360

Merged
merged 20 commits into from
Jun 25, 2021

Conversation

with-heart
Copy link
Contributor

What:
This PR is an attempt to add TypeScript-related tooling and config required to complete the conversion from Flow to TypeScript.

First step was getting eslint to run without any configuration errors (which @rjdestigter was running into in #2359), which seems to working for the most part. Also created a root tsconfig.json and tsconfig files for each package which extend from the root, since all of the existing were already duplicating the same config.

Why:
#2358

How:

  • Created a root tsconfig.json which specifies the compilerOptions shared by all of the existing tsconfig.json files
    • Set include to ["packages", "scripts", "site", "test", "playgrounds"]
    • Removed a few rules specifiers already enabled by "strict": true
  • Created new and moved existing tsconfig.json files at root of each package
    • Specified extends for each to root tsconfig.json
    • Removed compilerOptions from all as they now inherit these from the root
    • Set include for each to "src"
  • Configured eslint to support ts
    • Added @typescript-eslint/parser & @typescript-eslint/eslint-config
    • Switched eslint-config-standard to eslint-config-standard-with-typescript
    • Set parserOptions.project to root tsconfig.json
    • Disabled no-use-before-define and enabled @typescript-eslint/no-use-before-define as warn
  • Added @babel/preset-typescript package and added to presets in babel.config.js

Checklist:

  • Documentation
  • Tests
  • Code complete
  • Changeset N/A?

I haven't even attempted to build anything yet or actually convert files to ts. It seems like a lot of the base tooling- and config-related stuff probably needs to be dealt with first in order to support the conversion of actual packages.

I'd like to expand the checklist to include remaining items to handle, but I need some feedback and to explore a little more to determine what else there is to do.

@changeset-bot
Copy link

changeset-bot bot commented Apr 27, 2021

⚠️ No Changeset found

Latest commit: 8210bea

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@with-heart with-heart changed the title Add TypeScript tooling Add TypeScript tooling and configs Apr 27, 2021
@with-heart with-heart mentioned this pull request Apr 27, 2021
22 tasks
@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 27, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8210bea:

Sandbox Source
Emotion Configuration

@rjdestigter
Copy link
Contributor

Nice! I rebased (locally) my PR off your branch. It looks like there is a conflict between ESLint and Prettier when defining types and interfaces:

ESLint will complain about commas and semicolons: Unexpected separator (;).eslint@typescript-eslint/member-delimiter-style

While prettier seems to think we have bad syntax when there is neither: (property) SerializedStyles.styles: string

image

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
babel.config.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
packages/babel-plugin/tsconfig.json Outdated Show resolved Hide resolved
@with-heart
Copy link
Contributor Author

@rjdestigter Yeah, I noticed that as well! I think it has to do with prettier being unsure which parser to use. Need to investigate more.

@with-heart
Copy link
Contributor Author

I made a few changes that moved this forward:

  • removed standard plugins
  • added import, react, and @typescript-eslint as eslint plugins
  • added an eslint files override for **/packages/**/*.tsx? that sets the prettier parser to typescript

This puts us at 38 errors and 61 warnings. The warnings are all for @typescript-eslint/no-use-before-define, and the errors are split between the parser being unable to deal with flow and parserOptions.project confusion for root *.js config files and **/__mocks__/*.js files. Not sure of the best way to resolve that last issue.

@with-heart
Copy link
Contributor Author

Hoping to take a look at the review comments later today.

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@with-heart
Copy link
Contributor Author

I made a few more changes this morning. build works fine and linting now results in 32 errors related to @typescript-eslint/eslint-parser being unable to parse flow.

If I add an overrides section for **/packages/**/*.jsx?" with "parser": "babel-eslint", this drops to only 5 linting errors related to flow. I don't know enough about flow` to know how to resolve those though, and I'm not sure if we'd want to include that override temporarily while the conversion to ts happens.

Thoughts?

@@ -0,0 +1,4 @@
{

Choose a reason for hiding this comment

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

Can't leave a comment on the diff but packages/babel-plugin-jsx-pragmatic/tsconfig.json being empty looks to be the cause for the Flow CI issue.

yarn.lock Outdated
version "4.2.4"
resolved "https://registry.yarnpkg.com/typescript/-/typescript-4.2.4.tgz#8610b59747de028fda898a8aef0e103f156d0961"
integrity sha512-V+evlYHZnQkaz8TRBuxTA92yZBPotr5H+WhQ7bD3hZUndx5tGOa1fuCgeSjxAzM1RiN5IzvadIXTVefuuwZCRg==

typescript@next:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you'll want to combine these typescript versions. Eventually the project will use TypeScript language features post-3.6.0 and dtslint might break on them if it's using the old version.

typescript@next, typescript@^4.2.4:

@with-heart with-heart changed the base branch from main to ts-migration May 11, 2021 16:40
@JoshuaKGoldberg JoshuaKGoldberg mentioned this pull request May 21, 2021
4 tasks
tsconfig.json Outdated
"target": "es5"
},
"include": ["packages", "scripts", "site", "test", "playgrounds"],
"exclude": ["node_modules"]
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg May 21, 2021

Choose a reason for hiding this comment

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

I've found myself doing this in my own integration branch to get TypeScript to not complain about all the $ExpectError dts tests:

Suggested change
"exclude": ["node_modules"]
"exclude": ["node_modules", "packages/*/types/test*"]

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess, more broadly, what is the strategy for .d.ts generation? Should a build step like build:types be added that runs tsc --declaration into types/ folders?

Copy link
Member

Choose a reason for hiding this comment

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

I guess, more broadly, what is the strategy for .d.ts generation? Should a build step like build:types be added that runs tsc --declaration into types/ folders?

This one is covered by Preconstruct which we are using to build all packages. You can check out example output containing TS declarations here:
https://unpkg.com/browse/@changesets/cli@2.14.0/dist/

@JoshuaKGoldberg
Copy link
Contributor

@with-heart I got a passing build on this separate PR: JoshuaKGoldberg#1

Now that the Prettier update is in master, If you merge master into ts-migration and ts-migration into your fork's add-ts-tooling I can send a PR from my fork to add-ts-tooling?

@with-heart
Copy link
Contributor Author

Welp, I've updated the branch but I was being mindless and rebased. My bad for throwing a wrench into things, wasn't really thinking.

@JoshuaKGoldberg
Copy link
Contributor

This all looks good either way 😄 , I'll get my branch updated hopefully end of this week!

@with-heart, other than updating JoshuaKGoldberg#1, is there anything you'd like me to do to help? I think once that's updated it would be ready to be merged in here?

@Andarist
Copy link
Member

@with-heart friendly 🏓 would you mind taking a look at what's up with CI?

@with-heart
Copy link
Contributor Author

with-heart commented Jun 25, 2021

I fixed the issue with the eslint parser for prettier and removed the per-package tsconfig.json files. Are you able to re-run the workflows for me @Andarist?

@Andarist Andarist merged commit 4912b82 into emotion-js:ts-migration Jun 25, 2021
@Andarist
Copy link
Member

@with-heart thank you for this! ❤️

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.

6 participants