Skip to content

fix output escaping #19

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

Merged
merged 1 commit into from
Jul 29, 2020
Merged

fix output escaping #19

merged 1 commit into from
Jul 29, 2020

Conversation

3axap4eHko
Copy link
Member

During a conversion process via command

jss convert style.css -f js -e cjs > style.js

escaping characters should be escaped:

#big-bang::before {
  content: "\ffff";
}

should became

#big-bang::before {
  content: "\\ffff";
}

@kof
Copy link
Member

kof commented Jul 9, 2020

Do you foresee any problems with other properties besides content, e.g. background image URLs?

@3axap4eHko
Copy link
Member Author

@kof every backslash should be escaped, otherwise, it's used as an escape character and escapes the next character. Does it make sense for you?

@kof
Copy link
Member

kof commented Jul 9, 2020

yeah, anyways I am gonna merge it and we will see

@kof
Copy link
Member

kof commented Jul 9, 2020

Actually, it only makes sense if the escaping character is inside of a string value, like in case of content. Other properties may have actual character escape, which you would unescape.

@kof
Copy link
Member

kof commented Jul 9, 2020

I think ultimately we should movecss parser we currently use to postcss parser and add value parser plugin, which will be context aware, otherwise this kind of fixes will be an infinite source of bugs.

@3axap4eHko
Copy link
Member Author

The other properties within the actual escape character are not affected because the escape character is not represented by a backslash.

@3axap4eHko
Copy link
Member Author

@kof Could you give me an example of values that you were mentioning before?

@kof
Copy link
Member

kof commented Jul 9, 2020

Literally any value that potentially is escaped, so the backslash is actually escaping and doesn't need unescaping. Can be content, can be url

@kof
Copy link
Member

kof commented Jul 9, 2020

content: "test \"bla\"";

@3axap4eHko
Copy link
Member Author

content: "test \"bla\"";

At this case, my fix transpiles it to '"test \\"bla\\""' and once you set a css rule it converts back to "test \"bla\"" as a js expression

@kof
Copy link
Member

kof commented Jul 12, 2020

yea but it is supposed to render test "bla" in the end, right?

@3axap4eHko
Copy link
Member Author

nope, just test it out on own example

@3axap4eHko
Copy link
Member Author

@kof ping 😃

@3axap4eHko
Copy link
Member Author

actually I can add this example too

@maksym-boytsov
Copy link

I faced the same problem, tested on my example, and for me, it works well.
@kof I think that current PR solving the escaping problem

@kof kof merged commit 731e8ce into cssinjs:master Jul 29, 2020
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.

3 participants