Skip to content
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

Implement some features missing for literal syntax #43

Merged
merged 29 commits into from
Sep 15, 2021
Merged

Conversation

futursolo
Copy link
Owner

@futursolo futursolo commented Sep 13, 2021

Closes #31.

I think it's more reasonable to wrap up the current work in favour of #40 which will deprecate most of work here.

This PR consists of the following changes (inline):

  • @keyframes are supported properly
  • Unknown at-rules are denied
  • @supports and @rules can now be nested inside of blocks
  • Added a StyleContext so styles can be printed with indentation

@futursolo
Copy link
Owner Author

Benchmark Results:

Benchmark Result
Parse Simple (10,000,000 iterations) 436ms
Macro (Literal) Simple (10,000,000 iterations) 20ms
Macro (Inline) Simple (10,000,000 iterations) 23ms
Parse Simple, No Cache (100,000 iterations) 455ms
Parse Complex (1,000,000 iterations) 1093ms
Macro (Literal) Complex (1,000,000 iterations) 483ms
Macro (Inline) Complex (1,000,000 iterations) 508ms
Parse Complex, No Cache (100,000 iterations) 446ms
Cached Lookup (1,000,000 iterations) 156ms
Cached Lookup, Big Sheet (100,000 iterations) 396ms
Mounting (2,000 iterations) 24ms

Baseline (master):

Benchmark Result
Parse Simple (10,000,000 iterations) 427ms
Macro (Literal) Simple (10,000,000 iterations) 20ms
Macro (Inline) Simple (10,000,000 iterations) 21ms
Parse Simple, No Cache (100,000 iterations) 314ms
Parse Complex (1,000,000 iterations) 1103ms
Macro (Literal) Complex (1,000,000 iterations) 433ms
Macro (Inline) Complex (1,000,000 iterations) 469ms
Parse Complex, No Cache (100,000 iterations) 365ms
Cached Lookup (1,000,000 iterations) 151ms
Cached Lookup, Big Sheet (100,000 iterations) 344ms
Mounting (2,000 iterations) 20ms

Copy link
Collaborator

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A big change in the end but a worthy one. I especially like the reworked error reporting in the inline macro as a nice bonus. Having a single RuleBlockContent is easy to read, but I am struggling a bit to understand the logic of when the output opens/closes output blocks. As long as nested qualified rules are not implemented, the combinatorial explosion from scss is avoided, so that's okay with me and it's not too far up on my agenda.

I think the design of @keyframes could be its own PR, cause its usage is quite a bit different from styles, e.g. we'd want to generate the animation name, those have to be unique for the document.

packages/stylist-core/src/ast/context.rs Show resolved Hide resolved
packages/stylist-core/src/ast/context.rs Outdated Show resolved Hide resolved
packages/stylist-core/src/ast/context.rs Show resolved Hide resolved
packages/stylist-core/src/ast/context.rs Outdated Show resolved Hide resolved
packages/stylist-core/src/ast/rule.rs Show resolved Hide resolved
packages/stylist-macros/src/inline/parse/root.rs Outdated Show resolved Hide resolved
packages/stylist-macros/src/inline/parse/rule.rs Outdated Show resolved Hide resolved
packages/stylist-macros/src/inline/parse/scope.rs Outdated Show resolved Hide resolved
packages/stylist/src/ast.rs Outdated Show resolved Hide resolved
@futursolo
Copy link
Owner Author

futursolo commented Sep 14, 2021

I think the design of @Keyframes could be its own PR, cause its usage is quite a bit different from styles, e.g. we'd want to generate the animation name, those have to be unique for the document.

This PR only contains changes of reimplementing @keyframes with StyleAttr so that no existing test case would fail.

max-width: 500px;
}
}
@supports (max-width: 500px) {
Copy link
Owner Author

@futursolo futursolo Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's possible to deduplicate this @supports rule without having to implement a logic that also considers the next adjacent block / rule.

I am fine to overlook this bit without introducing more complicated logic into the StyleContext.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me aswell. At the end of the day, the output should resemble the input for easier debugging, but really only needs be correctly understood by the browser.

# Conflicts:
#	packages/stylist-core/src/ast/rule_content.rs
#	packages/stylist-macros/src/inline/parse/attribute.rs
#	packages/stylist-macros/src/literal/to_output_with_args.rs
#	packages/stylist-macros/src/output/block.rs
#	packages/stylist-macros/src/output/context.rs
#	packages/stylist-macros/src/output/maybe_static.rs
#	packages/stylist-macros/src/output/mod.rs
#	packages/stylist-macros/src/output/rule.rs
#	packages/stylist-macros/src/output/rule_content.rs
#	packages/stylist-macros/src/output/selector.rs
#	packages/stylist-macros/src/output/sheet.rs
#	packages/stylist-macros/src/output/str_frag.rs
#	packages/stylist-macros/src/output/style_attr.rs
#	packages/stylist/tests/macro_integrations/complicated-attributes-pass.rs
#	packages/stylist/tests/macro_integrations/nested_at_rule-pass.rs
@futursolo futursolo merged commit 2b96368 into master Sep 15, 2021
@futursolo futursolo deleted the rule-block branch September 15, 2021 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature parity between syntaxes
2 participants