Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Revamp custom property reference resolution
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