Skip to content

[WIP] fix: utf-8 characters support, emoji also 😸 #523

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

Closed

Conversation

alexander-akait
Copy link
Member

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?

yes

If relevant, did you update the README?

not required

Summary

Ref: #317

Also we should fix incorrect behvaior with utf-8 characters in postcss-modules-local-by-default. Let's leave this PR here, after how i resolve problem in postcss-modules-local-by-default we can merge this issue.

Does this PR introduce a breaking change?
no

Other information

@codecov
Copy link

codecov bot commented Apr 27, 2017

Codecov Report

Merging #523 into master will decrease coverage by 1.11%.
The diff coverage is 91.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
- Coverage   98.63%   97.52%   -1.12%     
==========================================
  Files          10       10              
  Lines         367      363       -4     
  Branches       87       89       +2     
==========================================
- Hits          362      354       -8     
- Misses          5        9       +4
Impacted Files Coverage Δ
lib/processCss.js 96% <91.42%> (-1.99%) ⬇️
lib/loader.js 98.61% <0%> (-1.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fee601...9222e8e. Read the comment docs.

@alexander-akait
Copy link
Member Author

alexander-akait commented Apr 27, 2017

/cc @michael-ciniawsky
why?

  1. css-selector-tokenizer don't works correctly with string, why they always escape this?
  2. css-selector-tokenizer don't supports non ASCII characters (national characters, emoji 😭 ).
  3. css-selector-tokenizer uses regex, it is bad, we should use parser.
  4. Using postcss-value-parser we take more flexible code.
  5. postcss-value-parser more faster then css-selector-tokenizer in most use case (just try to use console.time 😄 ).
  6. ...
  7. Profit

@michael-ciniawsky
Copy link
Member

👍 I'm all in removing it

  1. ...
  2. Profit

😆 🤣 lol...

@michael-ciniawsky michael-ciniawsky changed the title [do not merge] fix: utf-8 characters support, emoji also 😸. [WIP] fix: utf-8 characters support, emoji also 😸 Apr 27, 2017
@alexander-akait
Copy link
Member Author

@michael-ciniawsky
Copy link
Member

It makes sense when there is...

=> It makes no sense when there is... ?

Yep let's wait, what comes up there 🙃

@michael-ciniawsky
Copy link
Member

@evilebottnawi Still WIP 🙃 ?

@alexander-akait
Copy link
Member Author

alexander-akait commented May 15, 2017

@michael-ciniawsky This PR is ready to merge, but need patch in css-modules plugins (otherwise the error will remain), let's leave it here for now, i plan to solve the problem in near future.

@michael-ciniawsky
Copy link
Member

Ahh right you mentioned that 😛 I forgot sry yep kk

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented May 24, 2017

Can we close and fix it in css-modulesand develop? Ping @ sullenor or @ TrySound which are maintainers in css-modules now. Also maybe join https://gitter.im/css-modules/maintainers in case we need someone from css-modules :D

@alexander-akait
Copy link
Member Author

alexander-akait commented May 25, 2017

@michael-ciniawsky we should merge this and fix it in css-modules (if we fix it only in css-modules it this will leave the problem). Yes, we can fix this in css-modules, but we need add tests here after fix this in css-modules (don't close original issue, after add test we can close). Maybe join is good 😄

@alexander-akait
Copy link
Member Author

@TrySound i think you can help our with this 😄 Now we have css-selector-tokenizer in css-loader (also in postcss-modules-local-by-default, postcss-modules-scope and postcss-modules-resolve-imports). Should we rewrite everything on postcss-value-parser instead css-selector-tokenizer, or will correct errors in css-selector-tokenizer (now he doesn't support emoji and national language, also related to bug with content: "\F10C \F10D" mathiasbynens/cssesc#10)?

Bug here https://github.com/css-modules/css-selector-tokenizer/blob/master/lib/stringifyValues.js#L7 or maybe not bug, but it is not works as expected. You can find several issues with this behavior here.

@TrySound
Copy link
Contributor

@evilebottnawi Did you have failed tests?

@TrySound
Copy link
Contributor

I can't reproduce this problem because node.js can't parse this symbol

    fixture: '.foo { content: '😸 😸'; }'
                               ^
SyntaxError: Unexpected token ILLEGAL

@alexander-akait
Copy link
Member Author

@TrySound just take tests from here and run on current master branch, maybe someone has already fixed this, but the error was

@alexander-akait
Copy link
Member Author

@TrySound Original issue #317

@jo12bar
Copy link

jo12bar commented Jun 27, 2017

Is this still WIP? I keep running into the issue described in #526 when trying to use faceyspacey/extract-css-chunks-webpack-plugin.

@alexander-akait
Copy link
Member Author

@jo12bar yep, wip, in near future I'll finish it

@michael-ciniawsky
Copy link
Member

@evilebottnawi What's the status of this PR, should we land this before v1.0.0 ?

@alexander-akait
Copy link
Member Author

@michael-ciniawsky yes after 1.0.0

@Pro-Novice
Copy link

What kind of proposal do I submit for GSoC?

I have a few Node.js Applications .... Will that be of any help ?

@michael-ciniawsky
Copy link
Member

Status ? :)

@alexander-akait
Copy link
Member Author

Close in favor #748

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants