toStyles: don't mutate the tree argument #49806
Merged
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
|
Size Change: -21 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
jsnajdr
commented
Apr 14, 2023
| duotoneStyles.filter = styles.filter; | ||
| delete styles.filter; | ||
| } | ||
|
|
There was a problem hiding this comment.
@ajlende This behavior, where the duotone style is always extracted, and then used only conditionally, when the duotone selector exists, I think it was a bug originally introduced in #37727. It means that if there is a filter.duotone style specified, and no feature selector for it, then the style will be ignored. Instead of being applied to the root selector just like any other style. Not sure if this was ever intended.
The duotone selector was a first ever example of a "feature selector". And the generic feature selector feature was introduced only later. Today, it might be possible to remove the duotone-specific code completely and just use a generic feature selector for it. The only thing that needs to be done is to convert the legacy __experimentalDuotone option into a featured selectors.filter.duotone selector.
There was a problem hiding this comment.
Should we go ahead and actually do that?
There was a problem hiding this comment.
By "that" you mean eliminating the duotoneSelector variable and integrate it into featureSelectors? Yes, I could push a commit like this 👍
There was a problem hiding this comment.
yes, that's the idea.
Potentially we could add a "shim" within registerBlockType + a deprecated function call.
youknowriad
reviewed
Apr 18, 2023
| const duotoneStyles = {}; | ||
| if ( styles?.filter ) { | ||
| duotoneStyles.filter = styles.filter; | ||
| delete styles.filter; |
There was a problem hiding this comment.
Isn't there another fix where we avoid "delete" (avoid mutations at all)? It feels to me like if we keep it around, it can regress again at anytime with some small unintended changes.
There was a problem hiding this comment.
That would be very desirable and I spent some time thinking about how to do it. It wouldn't be a "fix" but rather a major rewrite of the toStyles function.
Currently it processes feature selectors and the root selector in separate loops. Pseudocode:
for ( featSelector of featureSelectors ) {
declarations = [];
for ( style of styles ) {
if ( styleUsesSelector( style, featSelector ) ) {
declarations.push( style );
delete styles[ style ];
}
}
write( `${ featSelector } { ${ declarations.join() } }`;
}
declarations = [];
for ( style of styles ) {
declarations.push( style );
}
write( `${ rootSelector } { ${ declarations.join() } }`;
The second loop processes only styles that haven't been deleted by the first loop.
Rewritten version would have one loop, assigning each style to the right selector:
declarations = new Map<string, string[]>();
for ( style of styles ) {
selector = findFeatureSelector( style ) || rootSelector;
declarations.get( selector ).push( style );
}
for ( [ selector, decls ] of declarations ) {
write( `${ selector } { ${ decls.join() } }` );
}
No delete needed.
Consider this PR a first step towards that goal. It's adding many new unit tests, which will be needed to perform the rewrite safely.
youknowriad
approved these changes
Apr 18, 2023
jsnajdr
force-pushed
the
update/to-styles
branch
from
4110706
to
412e004
Compare
Yes I can commit to that 🙂 |
bph
added
[Type] Bug
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.
Alternative to #49623 which doesn't clone the entire
treeobject, but only the style objects that are created bypickStyleKeys. Consumed styles are removed only when the "node" (returned bygetNodesWithStyles) has afeatureSelectorsorduotoneSelectorproperty, and for these nodes thestylesare always created withpickStyleKeys.I'm adding also several new unit tests, to cover various cases: feature selectors, block variation styles, duotone filters...