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

[SPIKE] Throw error if shorthand and longhand properties are mixed, or if invalid CSS properties are used #1747

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dddlr
Copy link
Collaborator

@dddlr dddlr commented Nov 28, 2024

What is this change?

Spike the following potential feature:

  • Throw a build-time error if shorthand properties and longhand properties are mixed in the same component.

As a bonus, I've also spiked the following feature, because it involves making mostly the same changes:

  • Throw a build-time error if an invalid CSS property is used in a component.

Why are we making this change?

While shorthand properties and longhand properties (e.g. margin and marginTop) are sorted deterministically in almost all cases due to #1700 , there are still some edge cases where this sorting cannot be preserved, causing shorthand properties to override longhand properties non-deterministically and vice versa.

For example, if an app imports two packages that use Compiled, but the app does not use Compiled's Webpack or Parcel plugin, we cannot guarantee that the styles of the two packages will remain sorted (because we have no control on how the bundler sorts the stylesheets).

There are several solutions to this:

  • Expand all shorthand properties to longhand properties, then use the runtime function ax to ensure that duplicate properties (e.g. margin-top: 5px and margin-top: 10px) are removed
    • This is impractical and a lot of effort, as the values developers use for CSS properties in Atlassian codebases can be dynamic or very complex to statically analyse
    • Some CSS shorthand properties have a very flexible syntax, and thus are an absolute pain to expand
  • Stop the root problem - ban mixing shorthand and longhand properties
    • Mixing is uncommon and is often done non-intentionally.
    • We take this approach here.

How are we making this change?

Build time error. We use a build time error, because build time and runtime are the only times where we can see all of the Compiled styles that will be applied to a given component. Doing this check at runtime is feasible (albeit a bit tricky due to the use of hashed class names) by extending the ax function, but ideally we'd like to not introduce any runtime impact if possible.

Limitations

This DOES NOT work when using styled composition (which is deprecated anyway):

import { styled } from '@compiled/react';
const BaseComponent = styled.div({ marginTop: '5px' });
const Component = styled(BaseComponent)({ margin: '2px' });

BaseComponent and Component are processed completely separately at build time.

We could make some changes to @compiled/babel-plugin to try to make Compiled "aware" of BaseComponent when processing Component, but it would probably be a bit hacky. Something to explore perhaps.

We could potentially extend ax to do a runtime check, but this would be even more hacky, as all that's passed to ax are the hashed class names. With this approach, we'd need to somehow go from the hashed class names back to the original CSS property names.


PR checklist

Don't delete me!

I have...

  • Updated or added applicable tests
  • Updated the documentation in website/
  • Changeset

Todos:

  • Verify this works with basic styled usages
  • Verify this works with css usages (including css composition)
  • Verify this works with complex styled usages (props, dynamic styles...)
  • Verify this works with cssMap
  • Verify this works with ClassName
  • Verify this works with xcss
  • See whether it's possible to run this check when using styled composition
  • Resolve TODOs in the source code of the PR
  • Add ability to configure exceptions for certain CSS properties, e.g. font (shorthand and longhand mixing is required for the Atlassian Design System font tokens)

Copy link

changeset-bot bot commented Nov 28, 2024

⚠️ No Changeset found

Latest commit: 255abf8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

netlify bot commented Nov 28, 2024

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

Name Link
🔨 Latest commit 255abf8
🔍 Latest deploy log https://app.netlify.com/sites/compiled-css-in-js/deploys/67490db0a038fc0008fd3daf

@dddlr dddlr force-pushed the feat/error-if-shorthand-longhand-mix branch from 23d35b5 to d25ee34 Compare November 28, 2024 06:02
@dddlr dddlr force-pushed the feat/error-if-shorthand-longhand-mix branch from ffbb4c2 to 7b99616 Compare November 29, 2024 00:00
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.

1 participant