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

Nested emoji class names do not work as expected #995

Closed
gera2ld opened this issue Nov 21, 2019 · 9 comments · Fixed by #998
Closed

Nested emoji class names do not work as expected #995

gera2ld opened this issue Nov 21, 2019 · 9 comments · Fixed by #998

Comments

@gera2ld
Copy link

gera2ld commented Nov 21, 2019

  • Operating System: Mac OS X
  • Node Version: 11.15.0
  • NPM Version: 6.7.0
  • webpack Version: 4.41.2
  • css-loader Version: 3.2.0

Expected Behavior

Emoji in nested class names should always work.

Actual Behavior

Nested emoji class names do not work as expected. See explanation below.

Code

I made a project for minimum reproduction: https://github.com/gera2ld/css-loader-bug

// webpack.config.js
// here is the css-loader part
      {
        test: /\.module\.css$/,
        use: {
          loader: 'css-loader',
          options: {
            modules: {
              localIdentName: '[emoji]',
            },
          },
        },
      },

And the CSS file style.module.css:

.a {
  color: red;
}
.a .b {
  color: green;
}

How Do We Reproduce?

After yarn build, we will get CSS like this (embedded in dist/main.js):

".\\1F4FF {\n  color: red;\n}\n.\\1F4FF .\\1F366 {\n  color: green;\n}\n"

To make it look better:

.\1F4FF {
  color: red;
}
.\1F4FF .\1F366 {
  color: green;
}

Open dist/index.html, and we will find color: green does not work. Tested in Chrome and Firefox.
1

According to the W3C document:

A "real" space after the escape sequence must be doubled.

That is the problem. .\1F4FF .\1F366 is actually the same as .\1F4FF.\1F366 (without the white space), so it does not work. I tried adding an extra space, .\1F4FF .\1F366 (with two spaces) worked as expected.

@alexander-akait
Copy link
Member

alexander-akait commented Nov 21, 2019

Why do you use emoji as classes? It will be removed in next version, just for information, i will look on issue in near future

@gera2ld
Copy link
Author

gera2ld commented Nov 22, 2019

Because it's short and cool... Why will it be removed?

@alexander-akait
Copy link
Member

@gera2ld because it is unsafe in some situations, buggy and do not supported old browsers

@gfortaine
Copy link

@gera2ld There is no point in using emojis as classnames. This is not only an antipattern, moreover the corresponding fix is causing severe regressions on our business critical project.

@evilebottnawi Would you kindly remove it, please ?

@alexander-akait
Copy link
Member

@gfortaine remove [emoji] placholder?

@alexander-akait
Copy link
Member

@gfortaine if you have a bug after this patch please open new issue with reproducible test repo

@gfortaine
Copy link

@evilebottnawi after further investigations, we have clearly identified our annoying bugs coming from the upgrade of postcss-modules-scope to 2.1.1 We use bootstrap and it completely broke the CSS for glyphicons. I will try to set up a reproducible test repo. However, I was a bit surprised to have a such regression on a patch release (3.2.1).

@gfortaine
Copy link

@evilebottnawi We do confirm that this is a blocking point for us to upgrade css-loader (still stuck with last working version that is 3.2.0). Moreover, this request looks like totally no sense. It is a bad practice to use emoji as class names, it seems that nobody is doing this. How could we revert this, please ? The bug appears with Glyphicon Halfings from Bootstrap 3.

@alexander-akait
Copy link
Member

@gfortaine do not use emoji, it already deprecated and will be remove in the next major release (soon), because they are unstable and bad for cache

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 a pull request may close this issue.

3 participants