Skip to content

Avoid prefixing css vars - #302

Merged
thysultan merged 2 commits into
masterfrom
avoid-prefixing-css-vars
Sep 9, 2022
Merged

Avoid prefixing css vars#302
thysultan merged 2 commits into
masterfrom
avoid-prefixing-css-vars

Conversation

@Andarist

@Andarist Andarist commented Sep 9, 2022

Copy link
Copy Markdown
Collaborator

fixes #301

@Andarist
Andarist requested a review from thysultan September 9, 2022 09:40
Comment thread src/Parser.js
@coveralls

coveralls commented Sep 9, 2022

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 50eaf8637b4ed9b8ed9bf6e49adf6cc35f4af1ca-PR-302

  • 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 98.835%

Totals Coverage Status
Change from base Build 08b179da63fe6f06ffc2ca9a166bbc5003366c33: 0.004%
Covered Lines: 270
Relevant Lines: 272

💛 - Coveralls

Comment thread src/Prefixer.js Outdated
*/
export function prefix (value, length, children) {
switch (hash(value, length)) {
switch (charat(value, 0) != 45 && charat(value, 1) != 45 && hash(value, length)) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

the expression's type now is false | number - perhaps we should convert false to a 0? would that have any effect on the engine? According to some TS PRs that I've seen lately (like this one) JS engines don't do jump-table optimization for switch statements anyway so perhaps this doesn't matter at all? I'm not sure 🤷‍♂️

@thysultan thysultan Sep 9, 2022

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The second charat(value, 0) != 45 is probably not needed as we also prefer to not touch -webkit- already prefixed props.

It'd put charat(value, 0) < 46 ? 0 : ... inside the hash function.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It depends on the function - we believe the jump table optimization didn't happen because forEachChild was just very large.

Comment thread test/Prefixer.js
@thysultan
thysultan merged commit 55819ed into master Sep 9, 2022
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.

CSS variables should not be prefixed

4 participants