-
Notifications
You must be signed in to change notification settings - Fork 168
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
Revamp custom property reference resolution #44
Closed
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
compressed-size: runtime library Size change: +2.67 kB 🟡
View unchanged
|
necolas
requested changes
Mar 5, 2024
packages/react-strict-dom/tests/__snapshots__/css-test.native.js.snap-native
Outdated
Show resolved
Hide resolved
Regarding the flow libdefs, I am working to finish work on my PR which should make it work more reliably, even with your changes. |
vincentriemer
force-pushed
the
pr44
branch
2 times, most recently
from
March 8, 2024 21:49
4ea8fb8
to
beac22f
Compare
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`.
necolas
approved these changes
Mar 10, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and follow ups! LGTM
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 (defines the tokenization and parsing algorithms) and the CSS Typed OM (defines the object model).
The
CSSTokenizer
andCSSParser
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 theCSSStyleValue
base/abstract class (doesn't really do much itself) that gets implemented byCSSUnparsedValue
andCSSVariableReferenceValue
which are responsible for the parsing/resolution of custom property references.The spec 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 aCSSUnparsedValue
instance. ACSSUnparsedValue
is effectively a list whose elements are either astring
or aCSSVariableReferenceValue
. Resolving aCSSUnparsedValue
effectively becomes a case of replacing each instance of aCSSVariableReferenceValue
with either a string representation of what the variable was pointing to or an empty string. Once aCSSUnparsedValue
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 theparse
method again on the resulting string from aCSSUnparsedValue
and it should result in a concrete type since all thevar(
tokens have been removed.All of the infrastructure introduced above is leveraged by the
resolveVariableReferences
method incustomProperties.js
which where we walk the object map to turn theCSSUnparsedValue
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
andCSSUnparsedValue
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
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.
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. argb(255, 255, var(--unknown))
gets resolved torgb(255, 255, )
). What would y'all think about changing the approach to be: if there's a variable we can't resolve in aCSSUnparsedValue
, we don't promote theCSSUnparsedValue
to a string and then ignore any properties whose value are still aCSSUnparsedValue
.