fix: parser not being able to distinguish between legacy and modern formats #653
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.
For code review, commit 3
fix: parser not being able to distinguish between legacy and modern formats
is the core of what the PR title suggests. Commits 1 and 2 are related to the code this work touches, but themselves not essential for the commit 3. As such, I'd be completely fine with a request to drop them or split them off into a separate PR.Changes
refactor: correct parseArgument types
Avoid the
/** @type {ArgumentMeta} */ (meta)
type assertion and use/** @satisfies {ArgumentMeta} */ (meta)
instead to ensuremeta
conforms to theArgumentMeta
type.chore: add comma check to legacy syntax parsing check to be more robust
Make the parser check identifying whether a parameter is the alpha channel more robust by checking for the presence of commas in the color string when checking for legacy syntaxes. This also allows for the removal of the
name !== "color"
check as modern color syntaxes can't contain commas.fix: parser not being able to distinguish between legacy and modern formats
Validate the parsed CSS types for elements in color strings that are using functional syntax against the coordinate grammar defined in the corresponding color space. If a disallowed type is found in the parsed color, a
TypeError
is thrown.To actually distinguish legacy and modern syntaxes, the parser was updated to look for a named format with a
_legacy
suffix first (e.g. parsing anhsl
color will look forhsl_legacy
first and then forhsl
). This is not a good system as it's not flexible and because it relies on somewhat strangely named formats to be defined in the library. Ideally, color spaces define their grammar kind of like the specification does. The parser would identify the color space and then check if the parsed color matches any of the space's grammar(s) (e.g. for HSL one of legacy hsl/hsla or modern hsl/hsla). I also think it would be useful to distinguish between grammars (parsing) and formats (serialization). However, any further structural changes to the parsing logic would blow up the scope of this change which intends to fix the incorrect parsing of legacy HSL colors.Closes #648.
Notes
As stated in the commit messages, I'm not happy with the approach of the
_legacy
-suffixed formats. I'd prefer a comprehensive grammar definition separately from formats to be used to try and match a parsed color string.The grammars for the HSL color space could be defined like this (based on https://www.w3.org/TR/css-color-4/#funcdef-hsl):
I haven't worked on anything using it so I can't say how ergonomic this is, but it should have all the information needed to accurately parse all specified color variants. The optional
legacy
property (false
by default) on a grammar definition would be used to indicate the necessity of commas in the parameter list. Theoptional
property (false
by default) on a parameter definition would be used to mark a parameter as optional. Lastly,types
lists the allowed CSS types per parameter. This schema also allows for such specifics like legacy RGB colors only allowing<percentage>
or<number>
and not a mix of them.Since introducing anything like this would affect all color space definitions, this would be well out of scope for what I set out to fix in this change which is to fix the parsing of legacy HSL colors.