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 some false positives with ternary operators and cssMap #1781

Merged
merged 4 commits into from
Jan 8, 2025

Conversation

dddlr
Copy link
Collaborator

@dddlr dddlr commented Jan 8, 2025

What is this change?

Fix two edge cases:

  • When using the same properties across two css function calls through ternary operators, we should not get an ESLint violation.
import { css } from '@compiled/react';

const oldStyles = css({
  font: '...',
  fontWeight: '...',
});
const newStyles = css({
  font: '...',
  fontWeight: '...',
});
const someCondition = true;
export const EmphasisText = ({ children }) => <span css={[someCondition ? oldStyles : newStyles]}>{children}</span>;
  • When using the same properties across two keys in a cssMap function call, we should not get an ESLint violation.
import { cssMap } from '@compiled/react';

const styles = cssMap({
  warning: {
    borderColor: '...',
    borderTop: '...',
  },
  debug: {
    borderColor: '...',
    borderTop: '...',
  },
  normal: {
    borderColor: '...',
    borderTop: '...',
  },
});
export const EmphasisText = ({ children, appearance }) => <span css={styles[appearance]}>{children}</span>;

Note that there still remain a lot of other (pre-existing) edge cases -- this rule is really hard to make fully rigorous, sorry 🙇 For example, this will still cause a false positive:

import { css } from '@compiled/react';

const oldStyles = css({
  fontWeight: '...',
});
const newStyles = css({
  font: '...',
});

const someCondition = true;
export const EmphasisText = ({ children }) => <span css={[someCondition ? oldStyles : newStyles]}>{children}</span>;

Why are we making this change?

We've received an internal query about a false positive with css, and it looks like it's pretty easy to fix.


PR checklist

Don't delete me!

I have...

  • Updated or added applicable tests
  • Updated the documentation in website/
  • Added a changeset (if making any changes that affect Compiled's behaviour)

@dddlr dddlr added the bug 🐛 Something isn't working label Jan 8, 2025
@dddlr dddlr self-assigned this Jan 8, 2025
Copy link

changeset-bot bot commented Jan 8, 2025

🦋 Changeset detected

Latest commit: a0f1be1

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

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

netlify bot commented Jan 8, 2025

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

Name Link
🔨 Latest commit a0f1be1
🔍 Latest deploy log https://app.netlify.com/sites/compiled-css-in-js/deploys/677de066d884290008c048db

code: outdent`
import { cssMap } from '${imp}';
const styles = cssMap({
warning: {
borderColor: '...', // 2
Copy link
Collaborator Author

@dddlr dddlr Jan 8, 2025

Choose a reason for hiding this comment

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

the false positive, if depth in correct order for shorthand properties in the same bucket for cssMap AND if key not static test case has been moved to the valid section, as I've fixed the false positive

It's now called depth in correct order for shorthand properties in the same bucket for cssMap AND if key not static on line 423

@@ -303,26 +344,24 @@ const parseCssMap = (
// If we know what key in the cssMap function call to traverse,
// we can make sure we only traverse that.
if (property.key.type === 'Literal' && key === property.key.value) {
properties.push(...getObjectCSSProperties(context, property.value));
break;
return getObjectCSSProperties(context, property.value);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

slight optimisation

@@ -310,6 +392,54 @@ tester.run('shorthand-property-sorting', shorthandFirst, {
export const EmphasisText = ({ children }) => <span css={styles}>{children}</span>;
`,
},

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this false negative already existed in the rule, it's just that we didn't have a test case for it

Copy link
Collaborator

@kylorhall-atlassian kylorhall-atlassian left a comment

Choose a reason for hiding this comment

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

I won't try to mentally run through the union stuff, but tests and the code that's obvious makes sense to me!

@dddlr dddlr merged commit d75db85 into master Jan 8, 2025
14 checks passed
@dddlr dddlr deleted the bug/shorthand-property-sorting-fix-ternary-operators branch January 8, 2025 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants