Skip to content

Commit

Permalink
Revamp custom property reference resolution
Browse files Browse the repository at this point in the history
Apologies in advance for the long PR.

> [!IMPORTANT]
> The last section of this description has an RFC with some points that I'd like to resolve with y'alls input before landing

## Description

This PR marks the beginning of an overhaul to how the StyleX React Native shim parses/ resolves values to make it more flexible and robust. Here I've laid the ground work of this overhaul and use it to replace the previous implementation of CSS variable resolution.

I've taken a spec-informed approach (try to match the spec as much as it makes sense for our use case) to implementing a tokenizer, parser, and object model for CSS values. To do this I heavily referenced the specs for [CSS Syntax](https://drafts.csswg.org/css-syntax-3/) (defines the tokenization and parsing algorithms) and the [CSS Typed OM](https://drafts.css-houdini.org/css-typed-om-1/) (defines the object model).

The `CSSTokenizer` and `CSSParser` are largely implementation details in this work and all parsing should be triggered at a higher level abstraction in the object model. As for the object model this first PR defines the `CSSStyleValue` base/abstract class (doesn't really do much itself) that gets implemented by `CSSUnparsedValue` and `CSSVariableReferenceValue` which are responsible for the parsing/resolution of custom property references.

[The spec](https://drafts.css-houdini.org/css-typed-om-1/#reify-tokens) goes into a lot more detail but the long and short of how this works is that whenever you're parsing a value contains a `var(` token it should always result in a `CSSUnparsedValue` instance. A `CSSUnparsedValue` is effectively a list whose elements are either a `string` or a `CSSVariableReferenceValue`. Resolving a `CSSUnparsedValue` effectively becomes a case of replacing each instance of a `CSSVariableReferenceValue` with either a string representation of what the variable was pointing to or an empty string. Once a `CSSUnparsedValue` only contains string items you can just concatenate them all and then do all your other sort of transforms. In this PR I've only implemented the branch of the parsing logic for ones that contain variable references but in the future you'd just run the `parse` method again on the resulting string from a `CSSUnparsedValue` and it should result in a concrete type since all the `var(` tokens have been removed.

All of the infrastructure introduced above is leveraged by the `resolveVariableReferences` method in `customProperties.js` which where we walk the object map to turn the `CSSUnparsedValue` into a resolved string.

> [!NOTE]
> I frankly didn't focus too much on how this performs and was instead focusing on correctness so there are probably low-hanging-opportunities to improve perf including moving some of this work to a compiler/build step much like StyleX does on the web.

## Test Plan

A lot of this work thankfully already had test coverage so a large portion of the work was ensuring that the existing tests continue to pass — though I had to make *some* adjustments to the tests as some of the expectations were to my eyes incorrect. The majority of new tests were written for the `CSSTokenizer` which gets complete coverage over the basic cases.

I added a couple tests for the `CSSParser` and `CSSUnparsedValue` largely as a mechanism to debug why other pre-existing tests were failing but once I figured it out I felt like it would be a waste to get rid of them.

I lastly added a couple additional end-to-end-ish tests (in `css-test.native.js`) that cover some of the new use cases this PR enables.

## Request for Help/Comments

1) My changes (at least locally) seemed to have broken the flow libdef generation (the script still runs successfully but its output has flow errors) and I don't have even an inkling as to why so I'd really appreciate help debugging the issue.

2) We sort of had undefined behavior previously in regards to what we should do when we try to resolve a variable which isn't defined. Since we only handled values which solely contained a single variable we could just resolve it to `null` and let our property pruning logic handle ignoring the property in its entirety — but now when a variable could be embedded in a compound value those assumptions fall apart (e.g. a `rgb(255, 255, var(--unknown))` gets resolved to `rgb(255, 255, )`). What would y'all think about changing the approach to be: if there's a variable we can't resolve in a `CSSUnparsedValue`, we **don't** promote the `CSSUnparsedValue` to a string and then ignore any properties whose value are still a `CSSUnparsedValue`.
  • Loading branch information
vincentriemer authored and vincentriemer@vincentriemer-mbp.dhcp.thefacebook.com committed Mar 8, 2024
1 parent 5597943 commit beac22f
Show file tree
Hide file tree
Showing 13 changed files with 740 additions and 189 deletions.
81 changes: 81 additions & 0 deletions flow-typed/npm/postcss-value-parser_vx.x.x.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow strict
*/

type PostCSSValueASTNode =
| {
type: 'word' | 'unicode-range',
value: string,
sourceIndex: number,
sourceEndIndex: number
}
| {
type: 'string' | 'comment',
value: string,
quote: '"' | "'",
sourceIndex: number,
sourceEndIndex: number,
unclosed?: boolean
}
| {
type: 'comment',
value: string,
sourceIndex: number,
sourceEndIndex: number,
unclosed?: boolean
}
| {
type: 'div',
value: ',' | '/' | ':',
sourceIndex: number,
sourceEndIndex: number,
before: '' | ' ' | ' ' | ' ',
after: '' | ' ' | ' ' | ' '
}
| {
type: 'space',
value: ' ' | ' ' | ' ',
sourceIndex: number,
sourceEndIndex: number
}
| {
type: 'function',
value: string,
before: '' | ' ' | ' ' | ' ',
after: '' | ' ' | ' ' | ' ',
nodes: Array<PostCSSValueASTNode>,
unclosed?: boolean,
sourceIndex: number,
sourceEndIndex: number
};

declare interface PostCSSValueAST {
nodes: Array<PostCSSValueASTNode>;
walk(
callback: (PostCSSValueASTNode, number, PostCSSValueAST) => ?false,
bubble?: boolean
): void;
}

type PostCSSValueParser = {
(string): PostCSSValueAST,
unit(string): { number: string, unit: string } | false,
stringify(
nodes: PostCSSValueAST | PostCSSValueASTNode | PostCSSValueAST['nodes'],
custom?: (PostCSSValueASTNode) => string
): string,
walk(
ast: PostCSSValueAST,
callback: (PostCSSValueASTNode, number, PostCSSValueAST) => ?false,
bubble?: boolean
): void
};

declare module 'postcss-value-parser' {
declare module.exports: PostCSSValueParser;
}
3 changes: 2 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion packages/react-strict-dom/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
},
"dependencies": {
"@stylexjs/stylex": "0.5.1",
"css-mediaquery": "0.1.2"
"css-mediaquery": "0.1.2",
"postcss-value-parser": "^4.1.0"
},
"devDependencies": {
"@stylexjs/babel-plugin": "0.5.1"
Expand Down

This file was deleted.

6 changes: 3 additions & 3 deletions packages/react-strict-dom/src/native/stylex/CSSMediaQuery.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,14 @@ export class CSSMediaQuery {
}

query: string;
matchedStyle: { [string]: mixed };
matchedStyle: { +[string]: mixed };

constructor(query: string, matchedStyle: { [string]: mixed }) {
constructor(query: string, matchedStyle: { +[string]: mixed }) {
this.query = query.replace(MQ_PREFIX, '');
this.matchedStyle = matchedStyle;
}

resolve(matchObject: MatchObject): { [string]: mixed } {
resolve(matchObject: MatchObject): { +[string]: mixed } {
const { width, height, direction } = matchObject;
const matches = mediaQuery.match(this.query, {
width,
Expand Down
106 changes: 106 additions & 0 deletions packages/react-strict-dom/src/native/stylex/customProperties.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow strict
*/

import { CSSUnparsedValue } from './typed-om/CSSUnparsedValue';
import { CSSVariableReferenceValue } from './typed-om/CSSVariableReferenceValue';

export type MutableCustomProperties = { [string]: string | number };
export type CustomProperties = $ReadOnly<MutableCustomProperties>;

function camelize(s: string): string {
return s.replace(/-./g, (x) => x.toUpperCase()[1]);
}

function normalizeVariableName(name: string): string {
if (!name.startsWith('--')) {
throw new Error("Invalid variable name, must begin with '--'");
}
return camelize(name.substring(2));
}

export function stringContainsVariables(input: string): boolean {
return input.includes('var(');
}

function resolveVariableReferenceValue(
propName: string,
variable: CSSVariableReferenceValue,
propertyRegistry: CustomProperties
) {
const variableName = normalizeVariableName(variable.variable);
const fallbackValue = variable.fallback;

let variableValue: string | number | null = propertyRegistry[variableName];

// Perform variable resolution on the variable's resolved value if it itself
// contains variables
if (
typeof variableValue === 'string' &&
stringContainsVariables(variableValue)
) {
variableValue = resolveVariableReferences(
propName,
CSSUnparsedValue.parse(propName, variableValue),
propertyRegistry
);
}

if (variableValue != null) {
return variableValue;
} else if (fallbackValue != null) {
const resolvedFallback = resolveVariableReferences(
propName,
fallbackValue,
propertyRegistry
);
if (resolvedFallback != null) {
return resolvedFallback;
}
}

console.error(
`React Strict DOM: Unrecognized custom property "${variable.variable}"`
);
return null;
}

// Takes a CSSUnparsedValue and registry of variable values and resolves it down to a string
export function resolveVariableReferences(
propName: string,
propValue: CSSUnparsedValue,
propertyRegistry: CustomProperties
): string | number | null {
const result: Array<string | number> = [];
for (const value of propValue.values()) {
if (value instanceof CSSVariableReferenceValue) {
const resolvedValue = resolveVariableReferenceValue(
propName,
value,
propertyRegistry
);
if (resolvedValue == null) {
// Failure to resolve a css variable in a value means the entire value is unparsable so we bail out and
// resolve the entire value as null
return null;
}
result.push(resolvedValue);
} else {
result.push(value);
}
}

// special case for signular number value
if (result.length === 1 && typeof result[0] === 'number') {
return result[0];
}

// consider empty string as a null value
const output = result.join('').trim();
return output === '' ? null : output;
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
*/

type InlineStyle = {
[key: string]: mixed
+[key: string]: mixed
};

type StylesArray<+T> = T | $ReadOnlyArray<StylesArray<T>>;
Expand Down
Loading

0 comments on commit beac22f

Please sign in to comment.