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

Rewrite to TypeScript #2358

Open
11 of 22 tasks
Andarist opened this issue Apr 26, 2021 · 26 comments
Open
11 of 22 tasks

Rewrite to TypeScript #2358

Andarist opened this issue Apr 26, 2021 · 26 comments

Comments

@Andarist
Copy link
Member

Andarist commented Apr 26, 2021

Since Flow is not actively used by any core contributor and its adoption in the community has dropped significantly (especially in comparison to its alternative) it's time to refactor the codebase from Flow to TypeScript.

We could use some help with that. It's best to start from the small, leaf packages. We already have TS types for our packages, they are just not generated from the source.

Progress

  • babel-plugin
  • babel-plugin-jsx-pragmatic
  • babel-preset-css-prop
  • cache
  • css
  • css-prettifier
  • eslint-plugin
  • hash
  • is-prop-valid
  • jest
  • memoize
  • native
  • primitives
  • primitives-core
  • react
  • serialize
  • server
  • sheet
  • styled
  • unitless
  • utils
  • weak-memoize
@iChenLei
Copy link
Contributor

Cooool ! This is a big migration work, we need a detailed roadmap to implement it.

@Andarist
Copy link
Member Author

For now, I would try to convert a simple package like @emotion/memoize to see what exact steps will be required there. When we know that then similar steps will have to be replicated in other packages.

@rjdestigter
Copy link
Contributor

I took a stab at the utils package: #2359

@with-heart
Copy link
Contributor

I opened #2360 to try to handle some of the tooling/config stuff needed to support the rest of the conversions. As of right now, it's mostly focused on tsconfig.json files and eslint config stuff.

eslint is able to auto-fix quite a few of the existing issues (didn't include the auto-fixes in the PR yet to keep it readable), but there are still 155 errors and 62 warnings. I need to go through those and figure out what's caused by missing config stuff versus what's caused by flow usage/will be resolved by actual TypeScript conversion.

@JoshuaKGoldberg
Copy link
Contributor

I would be honored to help with this! 🙌

Is there a list anywhere of what's in scope, and/or recommendations about what to start with?

@Andarist
Copy link
Member Author

@JoshuaKGoldberg it turned out that I have posted this issue at the totally wrong time for myself 😂 The last 2 weeks were super crazy for me and I didn't have time to sit down properly to review the 2 existing PRs for this that have started working on migrating some packages.

I think @with-heart's PR is currently the closest to getting the tooling-related stuff done: #2360 but there is still some work there to be done. If you could take a look into it - that would be highly appreciated. If no - I would wait a couple of days before we figure this out and land that PR. When that will be done it should be fairly easy to start migrating package after package - starting from "leaf" ones.

@Andarist
Copy link
Member Author

Andarist commented May 11, 2021

I've created a ts-migration branch. It seems to me that the easiest approach to do this would be to actually:

  1. drop Flow entirely - from the source code, CI configs, tooling
  2. add TS tooling (partially already done in Add TypeScript tooling and configs #2360 and TypeScript migration of @emotion/utils #2359
  3. migrate packages one by one

This would simplify both the whole process and individual PRs.

@michaeljaltamirano
Copy link

I've created a ts-migration branch. It seems to me that the easiest approach to do this would be to actually:

  1. drop Flow entirely - from the source code, CI configs, tooling
  2. add TS tooling (partially already done in Add TypeScript tooling and configs #2360 and TypeScript migration of @emotion/utils #2359
  3. migrate packages one by one

This would simplify both the whole process and individual PRs.

I think this is a great idea 🙂 Thanks for setting it up!

@JoshuaKGoldberg JoshuaKGoldberg mentioned this issue May 21, 2021
4 tasks
@JoshuaKGoldberg
Copy link
Contributor

  1. drop Flow entirely - from the source code, CI configs, tooling

I've started a PR in parallel to do so against the ts-migration branch here: #2385

If a maintainer could grant my builds to run, that'd be very helpful. 😄

@Andarist
Copy link
Member Author

If a maintainer could grant my builds to run, that'd be very helpful. 😄

I hate that GitHub "feature" >.< Can I somehow grant you permission to run builds? Or will I have to approve them manually until we merge your PR? I believe that the restrictions get lifted if you already have some commits in the repo.

@sarayourfriend
Copy link
Contributor

I'd also love to help with this effort. What's the next best package to tackle for this?

@Andarist
Copy link
Member Author

@sarayourfriend @emotion/sheet, @emotion/unitless, @emotion/memoize, @emotion/weak-memoize and @emotion/is-prop-valid are the easiest picks right now

@sarayourfriend
Copy link
Contributor

Just noting that is-prop-valid will be blocked by memoize.

@sarayourfriend
Copy link
Contributor

I opened several PRs for this. @Andarist any chance they could get merged sometime soon?

@Andarist
Copy link
Member Author

Yes, definitely! Thanks a lot for your contributions. I've seen all of them but I'm being slammed lately with work and didn't find the time to sit down to this in the late evenings (the only time I currently have for OSS). Sorry for the delay

@sarayourfriend
Copy link
Contributor

Okay, no worries! Just wanted to make sure it didn't slip through the cracks 🙂 Let me know if you need anything from me in the meantime!

@Beraliv
Copy link

Beraliv commented Sep 29, 2021

Hey!

If you still need help, please let me know where I can help! 👋

@Andarist
Copy link
Member Author

@Beraliv thank you for the offer! It feels like we should now take care of @emotion/cache and @emotion/serialize, after that we can try to convert @emotion/css and @emotion/server, which would bring us to the grand finale: @emotion/react and @emotion/styled. There are some other packages - like Babel plugins, ESLint plugin, and @emotion/jest but they are less important for now and they could be taken care of even after landing the TS migration on the main branch.

Note that the current progress is available on the ts-migration branch and PRs that got merged to it can serve as an example of how a migration PR should roughly look like. A single PR should, preferably, focus on a single package.

@danilofuchs
Copy link
Contributor

Hi, I would like to help migrating cache or serialize.
A merge from master would be required so we don't migrate stale code or introduce regressions.

Migrating everything at once in a single ts-migration branch is a huge task and the branch will be out of date very quickly.

Could we somehow merge the current work while keeping Flow enabled for the non-migrated packages?

@Andarist
Copy link
Member Author

Andarist commented Nov 4, 2021

Probably not that much has changed on the main branch since the last merge - i will merge it to the ts-migration soon. Feel free to migrate anything and just branch off the ts-migration branch. I will take care of the merge conflicts

@srmagura
Copy link
Member

In the packages that have already been migrated, we still have .d.ts files, tslint.json, and type checking tests. Is it safe to remove all of this stuff?

image

By the way, I am planning on working on some of the TypeScript tooling:

  • The tsconfig.json in each package will extend a top-level tsconfig.json. This keeps the configs consistent across packages.
  • Fix ESLint configs as there are currently configuration errors when trying to run lint
  • Remove the workflows that run dtslint (depending on the answer to my above question)
  • Add tsc script to the root package.json that type checks everything
  • Maybe other stuff 🤔

@G-Rath
Copy link
Contributor

G-Rath commented Nov 23, 2021

👋 I'm about to attempt a PR updating eslint-plugin to support ESLint v8, and saw this - I've done a fair amount of work in TS, including writing and converting ESLint plugins to TypeScript (I did it for eslint-plugin-jest); would folks like me to take a stab at converting @emotion/eslint-plugin?

I'm not sure when I'll get around to it, but I'm also happy to be pinged to help out if folks want :)

@Andarist
Copy link
Member Author

In the packages that have already been migrated, we still have .d.ts files, tslint.json, and type checking tests. Is it safe to remove all of this stuff?

This is still useful for testing the types - the index.d.ts and tslint.json are just enforced by the dtslint that we are using for the testing.

The tsconfig.json in each package will extend a top-level tsconfig.json. This keeps the configs consistent across packages.

Let's keep a single, top-level, config for the time being. It's how the setup works in some other repos that I collaborate on, like Changesets

Remove the workflows that run dtslint (depending on the answer to my above question)

The main advantage of using dtslint is that it runs those tests against multiple versions of TS - so I think we need to stick with it. However, this tool needs upgrading, see here

Add tsc script to the root package.json that type checks everything

This should be done - good catch (dtslint might not test our source files and it definitely wont test the test files)

👋 I'm about to attempt a PR updating eslint-plugin to support ESLint v8, and saw this - I've done a fair amount of work in TS, including writing and converting ESLint plugins to TypeScript (I did it for eslint-plugin-jest); would folks like me to take a stab at converting @emotion/eslint-plugin?

@G-Rath if you could take a stab at converting this, it would be great. I've never worked with ESLint rules written in TS so this would be of a great help.

@danilofuchs
Copy link
Contributor

danilofuchs commented Nov 23, 2021

Keeping the dtslint tests during migration is very useful to make sure the TS public interface is not being changed in the process. I myself caught a missing export when migrating serialize because of it.

If we only change the internal implementation from JS to TS, it should be transparent to end users, as many already use the currently exported TS types. After the migration is complete and stable, it might make sense to remove it.

Checking for multiple versions of TS is useful, might be worth keeping dtslint for it.

@srmagura
Copy link
Member

Currently yarn build (preconstruct) is creating .d.ts in the src directory of various packages. I'm pretty sure these should not be committed since they are compiled output, so I have just been careful not to add them when using git add.

Is there a way to prevent preconstruct from generating these unwanted files? @mitchellhamilton

@abreel
Copy link

abreel commented Aug 23, 2023

Hello @Andarist,
Would there be anything I could help with? First time contributor.

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

No branches or pull requests