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

[sheet] Convert to TypeScript #2431

Merged
merged 7 commits into from
Aug 14, 2021

Conversation

sarayourfriend
Copy link
Contributor

@sarayourfriend sarayourfriend commented Jul 13, 2021

What: Converts sheet to TypeScript

Why: Part of #2358

How: Renamed index.js to index.ts and then added back type definitons. Most of them were the same as the flow type save some void -> undefined. Part of that also changed the signature for sheetForTag to return undefined (it technically could) so we need to add a null check. I decided to create a utility called assertDefined similar to the one used by WordPress's dom package: https://github.com/WordPress/gutenberg/blob/af7c07ce630dff9177dd1e25c1459d7e25e836e2/packages/dom/src/utils/assert-is-defined.ts

This allows us to just call assertDefined and have a runtime check that the variable is defined. This runtime check is disabled in production to prevent a performance regression.

I also had to add the private _alreadyInsertedOrderInsensitiveRule attribute to the StyleSheet class. _insertTag is also marked private to match the original .d.ts files.

Also had to update one of the type tests that was erroring out. Not sure why it passed previously as options 100% needs to be defined or the constructor for StyleSheet will throw a TypeError (it doesn't check at all that options is defined).

Overall I think this PR actually adds some type safety/sanity to sheet that flow was missing.

Checklist:

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

Will depend on changes from #2427 for the jest tests to run by figured I'd get it ready anyway

@changeset-bot
Copy link

changeset-bot bot commented Jul 13, 2021

🦋 Changeset detected

Latest commit: 19a5074

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

This PR includes changesets to release 1 package
Name Type
@emotion/sheet 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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 13, 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 19a5074:

Sandbox Source
Emotion Configuration


private _alreadyInsertedOrderInsensitiveRule: boolean | undefined

constructor(options: Options) {
Copy link
Member

Choose a reason for hiding this comment

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

the previous definition was:

constructor(options?: Options);

which was incorrect because options are required.

I'm glad that the whole migration can fix some minor issues like this as isn't done only for the sake of migrating 🎉

@Andarist Andarist merged commit 52aadc6 into emotion-js:ts-migration Aug 14, 2021
@Andarist
Copy link
Member

@sarayourfriend thank you for all your hard work on those migrations PRs ❤️ I'm sorry that I didn't get to them sooner - which usually is not a problem - but I have had much less time for OSS lately and my backlog has overwhelmed me.

@sarayourfriend
Copy link
Contributor Author

@Andarist No worries, I totally understand! Glad I can help 😄

@sarayourfriend sarayourfriend deleted the ts-migration-sheet branch August 15, 2021 18:29
@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.

2 participants