compiled icon indicating copy to clipboard operation
compiled copied to clipboard

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

Open dddlr opened this issue 1 year ago • 2 comments

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 https://github.com/atlassian-labs/compiled/pull/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:

  • [x] Verify this works with basic styled usages
  • [x] 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)

dddlr avatar Nov 28 '24 05:11 dddlr

⚠️ No Changeset found

Latest commit: 255abf8ecf95fc5a36b79a03db7360d502b180a3

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

changeset-bot[bot] avatar Nov 28 '24 05:11 changeset-bot[bot]

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

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

netlify[bot] avatar Nov 28 '24 05:11 netlify[bot]

closing stale PRs, feel free to copy this over to a new one if needed!

dddlr avatar Jul 11 '25 04:07 dddlr