Fix composition of styles without a final semicolon in a declaration block

[Andarist]

fixes ##1284

It's a low-cost fix for what is a valid CSS ¯\(ツ)/¯

[changeset-bot]

🦋 Changeset is good to go

Latest commit: 0540a6b

We got this.

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

[codecov]

Codecov Report

Merging #1560 into master will not change coverage.
The diff coverage is 100%.

Impacted Files	Coverage Δ
packages/serialize/src/index.js	100% <100%> (ø)	⬆️

[emmatown]

Thanks!!

[damusnet]

Hi @Andarist

Could it be that this PR got too aggressive and is adding too many semi-colons? The dependency upgrade broke a lot of our snapshot tests by adding extra unnecessary ones, like this:

(see text-align:center;;label)

related code:

            <Modal
              containerStyle={css({
                width: WIDTH,
                display: 'flex',
                flexDirection: 'column',
                alignItems: 'center',
                textAlign: 'center',
              })}
              titleText={title}
              onExit={routerPush({ url })}
            >
              Content
            </Modal>

It seems to be happening only in components where we pass css({ ... }) as a prop from one parent component to a child, and I guess we can just update the snapshots and move on, but I was wondering if this was the expected behavior, or if maybe we are doing something wrong.

I tried doing a repro in CodeSandbox but they don't support jest.mock and I think this is where it happens 😕 https://codesandbox.io/s/long-wood-yr09n

[Andarist]

Yes - this is intentionally aggressive. Detecting if we actually need to add semi would be more costful than it is worth it - especially that an extra semi doesn't affect generated styles. What you see is an opaque object (is not expected to be stable/inspectable) and you really shouldn't test against it - you can use jest-emotion package to get more stable snapshots.

Technically after this PR got merged in maybe we could tweak the logic introduced in #1554 but it's a microoptimization at best. I'll look into it - but it's not a high priority right now.

[damusnet]

Thanks for your reply @Andarist ; I understand the reasoning.

FWIW we already use jest-emotion and I believe those opaque objects only happen when a css style is passed as a prop to a jest mocked component. Since the component is mocked, the styles to get expanded, and remain an object on the snapshot.

We'll update our snapshots.