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

fix(compiler-sfc): escape non-word characters of css var #6816

Merged
merged 3 commits into from Nov 9, 2022

Conversation

sxzz
Copy link
Member

@sxzz sxzz commented Oct 3, 2022

This bug will affect dev mode only.

closes #6803


ATM, #6815 hasn't been merged. So reproduction cannot reproduce on SFC playground.

Reproduction:

<script setup>
import { ref } from 'vue'

const 蓝色 = ref('blue')
const 字体 = ref(800)
</script>

<template>
  <div class="text">
    hello
  </div>
</template>

<style>
  .text {
    color: v-bind(蓝色);
    font-weight: v-bind(字体);
  }
</style>

@antfu
Copy link
Member

antfu commented Oct 4, 2022

Maybe we could keep replacing symbols to _ but escape CJK chars for better readability.

@sxzz
Copy link
Member Author

sxzz commented Oct 4, 2022

but it will cause conflict if there is the same length of symbols.

Example

@antfu
Copy link
Member

antfu commented Oct 4, 2022

We should always keep track of existing ones and add number suffixes to avoid conflicts.

@sxzz
Copy link
Member Author

sxzz commented Oct 4, 2022

Maybe we could keep replacing symbols to _ but escape CJK chars for better readability.

@antfu IMHO, I don't think it's a good solution. Matching CJK is a complicated feature and it's unnecessary to have this in vue core. To avoid adding complexity, I guess just to escape all non-word characters is enough. After all, it only affects dev mode.

@antfu
Copy link
Member

antfu commented Oct 4, 2022

I think readability is important for dev otherwise we could use hashes as prod. Filtering is quite simple with /\p{Unified_Ideograph}/u

@sxzz
Copy link
Member Author

sxzz commented Oct 4, 2022

It can only match Chinese characters, but can't Japanese Kana, Korean Hangeul... There are lots of other language characters around the world, e.g. Russian alphabet, and the Greek alphabet. How can vue handle all of them?
image


Plus, I think escaping is acceptable, not very bad
image

@posva
Copy link
Member

posva commented Oct 6, 2022

can‘t we keep the unicode characters (in dev) as we do with variable names? After all, they seem to be valid CSS variable names

@sxzz
Copy link
Member Author

sxzz commented Oct 6, 2022

@posva I tried Chinese and Japanese characters, and it can work. So maybe can we skip escaping from Unicode characters?

@sxzz
Copy link
Member Author

sxzz commented Oct 6, 2022

I found an API CSS.escape, it can escape css var name perfectly. But it's browser-only API.

Can I add a polyfill dependency css.escape to compiler-sfc?

@posva
Copy link
Member

posva commented Oct 6, 2022

I think it's better to avoid adding any polyfill. If Unicode works and is specified in the spec, let's do that. It should also be the lightest solution

@sxzz
Copy link
Member Author

sxzz commented Oct 6, 2022

@posva Updated. It only escapes ASCII punctuation and symbols:  !"#$%&'()*+,./:;<=>?@[\]^`{|}~

@sxzz sxzz changed the title fix(compiler-sfc): escape non-word characters fix(compiler-sfc): escape non-word characters of css var Oct 7, 2022
posva
posva approved these changes Oct 18, 2022
Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

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

Despite devtools not highlighting the syntax correctly: the ) looks like it closes the var() statement

Screenshot 2022-10-18 at 12 32 25
😆

It looks good! 👍

Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

LGTM. Giving the fact that and . are quite common, maybe we could replace them to _ for better readbility?

antfu
antfu approved these changes Oct 28, 2022
@sxzz sxzz force-pushed the fix/css-var branch 2 times, most recently from 577d166 to 8123c37 Compare November 1, 2022 16:46
@netlify
Copy link

netlify bot commented Nov 1, 2022

Deploy Preview for vuejs-coverage failed.

Name Link
🔨 Latest commit 8123c37
🔍 Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/63614d6b7079f200089a7d4f

@sxzz sxzz force-pushed the fix/css-var branch 3 times, most recently from 340ef42 to f719525 Compare November 8, 2022 10:29
@yyx990803 yyx990803 merged commit 57c9013 into vuejs:main Nov 9, 2022
8 of 9 checks passed
@sxzz sxzz deleted the fix/css-var branch November 9, 2022 03:31
chrislone pushed a commit to chrislone/core that referenced this pull request Feb 4, 2023
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.

Unicode variable names in v-bind of Style will be convert to _
4 participants