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

tighten up cssMap type #1502

Merged
merged 10 commits into from
Sep 20, 2023
Merged

tighten up cssMap type #1502

merged 10 commits into from
Sep 20, 2023

Conversation

liamqma
Copy link
Collaborator

@liamqma liamqma commented Sep 1, 2023

Further tighten up cssMap type to prevent some invalid usages.

Examples
No Arrays

const styles = cssMap({
   foo: [{ color: 'red' }]
})

No tagged template

const styles = cssMap({
   foo: `
     color: red
   `
})

No arrow function

const styles = cssMap({
   foo: {
      color: (props) => props.a ? 'red' : 'green'
   }
})

Only string and number are valid CSS values. No boolean/undefined.

const styles = cssMap({
   foo: {
      color: possibleUndefined && 'red'
   }
})

@changeset-bot
Copy link

changeset-bot bot commented Sep 1, 2023

🦋 Changeset detected

Latest commit: 8e057a5

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

This PR includes changesets to release 6 packages
Name Type
@compiled/react Minor
@compiled/parcel-optimizer-test-app Patch
@compiled/parcel-transformer-test-app Patch
@compiled/parcel-transformer-test-compress-class-name-app Patch
@compiled/parcel-transformer-test-custom-resolver-app Patch
@compiled/parcel-transformer-test-extract-app 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

* }
* ```
*/
type CssObject = Readonly<
Copy link
Collaborator

@dddlr dddlr Sep 1, 2023

Choose a reason for hiding this comment

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

I wonder if there's a way to ensure that the user can't nest objects indefinitely or with invalid keys, e.g. stuff like

const styles = cssMap({
  danger: {
      veryDanger: {
        color: 'red',
      }
  },
})

without affecting valid use cases such as @media queries

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not at my laptop so I can't test this out, but it does seem like there's nothing stopping the user from doing stuff like

const styles = cssMap({
  danger: {
      veryDanger: {
        lotsofDanger: {
          evenmoreDanger: {
            spectacularDanger: {
              color: 'red',
            }
          }
        }
      }
  },
})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea, we've to allow it unless we impose stricter types.
The problem is that we don't know if the object key is a selector.
In the below example, we don't know if header is a variant name or a selector.

const styles = cssMap({
  success: {
      header: { // Is it <header /> or a variant name? 🤷‍♂️
         div: {
             color: 'red'
         }
      }
  },
})

I thought about disallowing targeting selectors completely, but I think disallowing it is too specific to Atlassian CSS standards.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, thank you for clarifying 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can do better here, it may be worth a play with the structure

I've made a quick spike based on vanilla extract types https://github.com/atlassian-labs/compiled/pull/new/css-map-type-spike

In https://vanilla-extract.style/documentation/styling/#media-queries, they specify some bounded keys that allow the types to be much stricter

dddlr
dddlr previously approved these changes Sep 4, 2023
JakeLane
JakeLane previously approved these changes Sep 4, 2023
@dddlr dddlr self-assigned this Sep 6, 2023
@dddlr dddlr marked this pull request as draft September 11, 2023 04:04
@dddlr
Copy link
Collaborator

dddlr commented Sep 11, 2023

Todo:

  • Rebase
  • Remove pseudos that we don't recommend using
  • Fix flow errors 😢
  • Double check type checking that nested selectors still work, as long as they're all targeting the same element
  • Unit tests - thoroughly test both StringLiteral and Identifier
  • Update VR tests
  • Investigate why having an empty object as a variant value gives an unhelpful error
  • Replace throw new Error with buildCodeFrameError
  • Make a PR updating website documentation
  • Update changeset to minor, document new selector restrictions and at-rule changes in changeset

@dddlr dddlr dismissed stale reviews from JakeLane and themself via bdf3f74 September 11, 2023 04:29
@dddlr dddlr added the developer experience 🏖️ It's something that improves the developer experience label Sep 11, 2023
| '&::spelling-error'
| '&::target-text'
| '&::view-transition'
| '&:active'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be great to reduce the total amount of declarations. So instead of allowing both ::active and :active we settle on one.

Copy link
Collaborator

@dddlr dddlr Sep 12, 2023

Choose a reason for hiding this comment

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

Ah good catch, didn't notice the duplicated pseudo-elements (:before/::before, :after/::after, :first-letter/::first-letter, :first-line, ::first-line)

@liamqma
Copy link
Collaborator Author

liamqma commented Sep 19, 2023

Looks good. I wanted to give approval but notice I cannot approve my own PR 😬.

dddlr
dddlr previously approved these changes Sep 19, 2023
@dddlr
Copy link
Collaborator

dddlr commented Sep 19, 2023

all good @liamqma i've approved it now (despite most of the changes being from myself 😆 )

@dddlr dddlr merged commit b6f3e41 into master Sep 20, 2023
5 checks passed
@dddlr dddlr deleted the fix-cssmap-type branch September 20, 2023 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer experience 🏖️ It's something that improves the developer experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants