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

fix(serialize): registered parameter of serializeStyles should be optional #2572

Merged
merged 3 commits into from
May 18, 2022

Conversation

otakustay
Copy link
Contributor

What:

According to its implementation and how css function uses it, the registered paramter of serializeStyles should be optional

Why:

To implement a custom css function working with frameworks other than react, current type is generating type erorrs like:

Expected 2-3 arguments, but got 1.ts(2554)
index.d.ts(67, 3): An argument for 'registered' was not provided.
(alias) serializeStyles<P>(args: (TemplateStringsArray | Interpolation<P>)[], registered: RegisteredCache, props?: P | undefined): SerializedStyles
import serializeStyles

How:

I added a single optional placeholder to this parameter, so that custom bindings like emotion-to-solid.js can work strong typed.

Checklist:

  • Documentation
  • Tests
  • Code complete
  • Changeset

@changeset-bot
Copy link

changeset-bot bot commented Nov 29, 2021

🦋 Changeset detected

Latest commit: 1266d78

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

This PR includes changesets to release 1 package
Name Type
@emotion/serialize Patch

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 Nov 29, 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 1266d78:

Sandbox Source
Emotion Configuration

@danilofuchs
Copy link
Contributor

@Andarist if this is correct, we must update the serialize TS migration as well

@otakustay
Copy link
Contributor Author

Is there anything I can do to make it merged?

@srmagura
Copy link
Member

Seems legit to me, @Andarist can you take a look at this?

@srmagura srmagura requested a review from Andarist May 18, 2022 14:46
@Andarist Andarist merged commit 5e81f21 into emotion-js:main May 18, 2022
@Andarist
Copy link
Member

Thanks ❤️ and sorry for not getting to this sooner

@github-actions github-actions bot mentioned this pull request May 18, 2022
srmagura added a commit to srmagura/emotion that referenced this pull request May 20, 2022
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.

None yet

4 participants