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.

Sign up for GitHub

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

Jump to bottom

Fix the cx() function's class collection at runtime as it generated class instances rather than strings. #1697

Merged
merged 2 commits into from
Jul 21, 2024
Merged

Fix the cx() function's class collection at runtime as it generated class instances rather than strings. #1697

merged 2 commits into from
Jul 21, 2024

Conversation

kylorhall-atlassian
Copy link
Collaborator

@kylorhall-atlassian kylorhall-atlassian commented Jul 18, 2024 •
edited
Loading

Alternative to #1696

Migrate from ac() which generates AtomicGroups classes to ax() which strictly generates strings (I think that's what this all does).

This is because passing <Component xcss={cx(style, style)}> around resulted in errors, 'cls.split is not a function' trying to iterate over non-strings.

Previously this generated classes collected into ac like:

[
  '_1bah1yb4',
  undefined,
  ['_16jlkb7n', undefined], // from `xcss={[…]}`, not accounted for in this PR, however, not real
  AtomicGroups { // from `xcss={cx(…)}`
    values: Map(2) { '_1bsb' => '_1bsb1osq', '_16jl' => '_16jlkb7n' }
  }
]

PR checklist

I have...

  • Updated or added applicable tests
  • Chosen not to document this in website/

@kylorhall-atlassian
Fix the cx() function's class collection at runtime as it generated…
02fc027 
… class instances rather than strings

Migrate from `ac()` which generates `AtomicGroups` classes to `ax()` which strictly collects strings.

This is because passing `<Component xcss={cx({ … })}>` around resulted in errors, `'cls.split is not a function'` trying to iterate over non-strings.
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jul 18, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: f0f1899

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

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

@netlify Netlify
Copy link

netlify bot commented Jul 18, 2024 •
edited
Loading

✅ Deploy Preview for compiled-css-in-js canceled.

Name Link
🔨 Latest commit f0f1899
🔍 Latest deploy log https://app.netlify.com/sites/compiled-css-in-js/deploys/6699bf8ffe440600087425c4

@kylorhall-atlassian kylorhall-atlassian mentioned this pull request Jul 18, 2024
Closed
2 tasks
kylorhall-atlassian
kylorhall-atlassian commented Jul 18, 2024
View reviewed changes
packages/react/src/xcss-prop/index.ts
@@ -208,5 +208,5 @@ export const cx = <TStyles extends [...XCSSProp<any, any>[]]>(

// The output should be a union type of passed in styles. This ensures the call
// site of xcss prop can raise violations when disallowed styles have been passed.
return ac(actualStyles) as TStyles[number];
return ax(actualStyles) as TStyles[number];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm worried this doesn't fail tests, not sure how to test this to be honest. I don't think anything is testing this properly (else it would've been failing before).

Copy link
Collaborator

Choose a reason for hiding this comment

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

a setup like that in packages/react/src/create-strict-api/__tests__/css-func.test.tsx could work for testing it right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought this would've failed in this test before to be frank:

compiled/packages/react/src/create-strict-api/__tests__/index.test.tsx

Line 526 in 39d9a02

const { getByTestId } = render(<Button xcss={cx(stylesOne.primary, stylesTwo.hover)} />);
, but I think because it was passing to the root <div className={xcss}> it wasn't a problem, it's when it's wrapped in <Component xcss={xcss}> it becomes a problem…?

Added tests in f0f1899 and can confirm, without this ax diff it breaks:
Screenshot 2024-07-19 at 1 18 44 PM

liamqma
liamqma previously approved these changes Jul 18, 2024
View reviewed changes
dddlr
dddlr previously approved these changes Jul 19, 2024
View reviewed changes
Copy link
Collaborator

@dddlr dddlr left a comment

Choose a reason for hiding this comment

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

lgtm as long as you're confident that cx will continue to work

@kylorhall-atlassian kylorhall-atlassian dismissed stale reviews from dddlr and liamqma via f0f1899 July 19, 2024 01:21
@kylorhall-atlassian kylorhall-atlassian marked this pull request as ready for review July 19, 2024 01:21
@kylorhall-atlassian kylorhall-atlassian merged commit 25a4bed into master Jul 21, 2024
15 checks passed
@kylorhall-atlassian kylorhall-atlassian deleted the fix-cx-class-collection branch July 21, 2024 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itsdouges itsdouges itsdouges approved these changes

@dddlr dddlr dddlr approved these changes

@liamqma liamqma liamqma left review comments

@JakeLane JakeLane Awaiting requested review from JakeLane JakeLane is a code owner

@pancaspe87 pancaspe87 Awaiting requested review from pancaspe87 pancaspe87 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kylorhall-atlassian @dddlr @liamqma @itsdouges