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

Change Array to ReadonlyArray in CSS type declarations #3141

Merged
merged 4 commits into from
Dec 23, 2023

Conversation

Cerber-Ursi
Copy link
Contributor

@Cerber-Ursi Cerber-Ursi commented Dec 15, 2023

What:

[email protected] changed the type of multi-valued fallback CSS properties from array to readonly array - frenic/csstype@46694de. This merge request follows that change, retyping ArrayCSSInterpolation from Array to ReadonlyArray.

Why:

This change is created in response to issue #3136.

Checklist:

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

Copy link

changeset-bot bot commented Dec 15, 2023

🦋 Changeset detected

Latest commit: b7312b8

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

Copy link

codesandbox-ci bot commented Dec 15, 2023

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 b7312b8:

Sandbox Source
Emotion Configuration

@freben
Copy link

freben commented Dec 22, 2023

@Cerber-Ursi Note that according to CONTRIBUTING.md you need to make a changeset to get things released.

And also we'd be grateful if you had the time to look into the build error.

@Andarist
Copy link
Member

We should also have a type-level test that will showcase that both mutable and readonly arrays can be used now

@Cerber-Ursi
Copy link
Contributor Author

you need to make a changeset to get things released.

Done. Misunderstood the rules - thought somehow that it's expected to be done when preparing the release (not just for any change in the release) and that it's expected to be done by the one actually releasing; I see now that this is incorrect.

And also we'd be grateful if you had the time to look into the build error.

It's actually somewhat cryptic. The problem is with this code:

// $ExpectError
css`
  position: relative;
  flexgrow: ${() => 20};
`

...and TypeScript throws the error on the interpolation, not on the css function call, so ExpectError isn't matched. Not sure why this worked before, to be honest.

We should also have a type-level test that will showcase that both mutable and readonly arrays can be used now

Added some fields to CSSObject value in test, thanks for noting this. Caught another place which had to change (where readonly array was not allowed).

@Cerber-Ursi
Copy link
Contributor Author

Managed to pull off the fix for build. Actually, this change seems to have slightly improved ergonomics for css template function.

For the record, here's the problem:

  • Before the change, if css-tagged template was called with incorrect interpolation, TypeScript checked it in the following way:
    • First overload, with TemplateStringsArray as the first argument, is incorrect, since the second argument is not a CSSInterpolation.
    • Second overload is incorrect, because it attempts to treat TemplateStringsArray as CSSInterpolation - and this fails, because TemplateStringsArray is a ReadonlyArray, and CSSInterpolation is a plain Array.
    • Therefore, it's the first argument to css function which TypeScript treats as mismatched - not the second.
    • Therefore, the whole css-tagged template literal was marked as erroneous.
  • Now, the check goes another way:
    • First overload is invalid, as before.
    • Second overload is invalid too, but now TemplateStringsArray as CSSInterpolation actually typechecks, and error is moved forward to the interpolated value.
    • Therefore, only the interpolated value is an error - not the whole literal.

This looks like a subtle unexpected improvement, since now the errors are more precise. But the negative part was that the old test - which had ExpectError on the literal - now fails. I've moved the ExpectError on the interpolation itself, and now test passes.

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