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

TypeScript migration of @emotion/utils #2359

Merged

Conversation

rjdestigter
Copy link
Contributor

What:
Migrating @emotion/utils from Javascript + Flow to TypeScript.

This was an initial stab at migrating a package to TypeScript. I'm primarily running into problems with linting. I had to add a line to .eslintignore due to ESLint not understanding optional properties on types and interfaces. This first commit also used --no-verify as the commit hook script also failed for similar reasons.

I kept the old declaration for now to use in the test.ts file to make sure that the auto generated function signatures matched the older declaration.

Build and tests do succeed on this branch atm.

Todo:

  • Figure out linting
  • Figure out proper TypeScript version?
  • Commit hook linting fails
  • Remove old declaration files
  • Is the TSLint file still relevant? Should the repo's ESLint configuration be updated first maybe?

Why:
#2358

How:

  • Added the latest TypeScript as a dependency to @emotion/utils
  • Moved existing tsconfig.json to root of package
  • Moved *.js files to *.ts and removed references to Flow

Checklist:

More todos:

  • Documentation N/A?
  • Tests (They are running atm)
  • Code complete
  • Changeset

@changeset-bot
Copy link

changeset-bot bot commented Apr 26, 2021

🦋 Changeset detected

Latest commit: 3c57837

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@emotion/utils Minor

Not sure what this means? Click here to learn what changesets are.

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

@rjdestigter rjdestigter mentioned this pull request Apr 26, 2021
22 tasks
@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 26, 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 1a93635:

Sandbox Source
Emotion Configuration

@Andarist
Copy link
Member

Thank you for starting this! I've already played a little bit with your branch locally and trying to wrestle the config-related stuff. It might take me some days - since I have to pause working on this and won't be available much in the following days but I will get back to this ASAP and get this through the finish line!


export type EmotionCache = {
inserted: { [string]: string | true },
inserted: Record<string, string | boolean>,

Choose a reason for hiding this comment

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

Would it be more consistent to keep the type literal true here?

Copy link

@MattSchiller MattSchiller left a comment

Choose a reason for hiding this comment

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

As a casual consumer of this repo, I love seeing this work starting! Left a comment and gonna see what I can do to contribute here, too 🙇

@@ -44,7 +43,7 @@ export const insertStyles = (
}
if (cache.inserted[serialized.name] === undefined) {
let stylesForSSR = ''
let current = serialized
let current: SerializedStyles | undefined = serialized

Choose a reason for hiding this comment

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

Is this even actually possible for serialized to be undefined here? I would've expected if (cache.inserted[serialized.name] === undefined) { to error from accessing undefined.name if so.

If that's actually possible, would it make sense to introduce optional chaining into the if statement's syntax?

Choose a reason for hiding this comment

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

I think it is current-specific typing in advance because current.next is possibly undefined and we're checking against it as part of the do/while:

let current: SerializedStyles | undefined = serialized
do {
let maybeStyles = cache.insert(
serialized === current ? `.${className}` : '',
current,
cache.sheet,
true
)
if (!isBrowser && maybeStyles !== undefined) {
stylesForSSR += maybeStyles
}
current = current.next
} while (current !== undefined)

Choose a reason for hiding this comment

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

Ah, gotcha, that makes sense. I haven't seen a while loop like this in a while :pun-dog: so it threw me. Thanks for the quick update!

@Andarist
Copy link
Member

Andarist commented Jul 8, 2021

@rjdestigter ive merged in TS tooling setup into ts-migration branch. If you would rebase on top of that it would be sweat. Let me know if you are still interested in working on that

# Conflicts:
#	.eslintignore
#	packages/utils/src/index.ts
#	packages/utils/src/types.ts
#	packages/utils/tsconfig.json
@Andarist Andarist changed the base branch from main to ts-migration August 15, 2021 08:12
@Andarist Andarist merged commit 16d8a8c into emotion-js:ts-migration Aug 15, 2021
@github-actions github-actions bot mentioned this pull request May 17, 2024
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.

4 participants