search

emotion-js / emotion

👩‍🎤 CSS-in-JS library designed for high performance style composition
https://emotion.sh/
MIT License
17.43k stars 1.11k forks source link

Rewrite to TypeScript #2358

Open Andarist opened 3 years ago

Andarist commented 3 years ago

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

iChenLei commented 3 years ago

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

Andarist commented 3 years ago

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 commented 3 years ago

I took a stab at the utils package: https://github.com/emotion-js/emotion/pull/2359

with-heart commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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: https://github.com/emotion-js/emotion/pull/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 commented 3 years ago

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 #2360 and #2359
  3. migrate packages one by one

This would simplify both the whole process and individual PRs.

michaeljaltamirano commented 3 years ago

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 #2360 and #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 commented 3 years ago
  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: https://github.com/emotion-js/emotion/pull/2385

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

Andarist commented 3 years ago

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 commented 3 years ago

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

Andarist commented 3 years ago

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

sarayourfriend commented 3 years ago

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

sarayourfriend commented 3 years ago

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

Andarist commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Hey!

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

Andarist commented 3 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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:

G-Rath commented 2 years ago

👋 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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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