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

values in selectors aren't escaped properly. #966

Closed
jquense opened this issue Jul 9, 2019 · 14 comments · Fixed by #973 or #1196
Closed

values in selectors aren't escaped properly. #966

jquense opened this issue Jul 9, 2019 · 14 comments · Fixed by #973 or #1196

Comments

@jquense
Copy link
Contributor

jquense commented Jul 9, 2019

on latest css-loader. I'd expect this to be true for any defined value but not importing classes where the local ident is generated.

// button.css

.button {

}

// toolbar.css

@value btn from './button.css'

.toolbar > btn {
	color: red
}
rules: [
  { 
  test: /\.css/, 
  use: {
    loader: 'css-loader',
    options: {
		modules: {
			getLocalIdent: (ctx, name) => `${name}.hey`
		}
	}
  }
]

The local names would need escaping and webpack outputs:

.button\.hey {
}

which is correctly escaped but in toolbar its

.toolbar\.hey > .button.hey {
}

note that the interpolated value isn't escaped. I'm not sure why this is happening maybe it needs to be re-escaped?

@alexander-akait
Copy link
Member

Feel free to send a PR

@jquense
Copy link
Contributor Author

jquense commented Jul 9, 2019

always happy to help out, I was hoping you might have an idea where the problem could be?

also why you are here what do you think about: css-modules/css-modules#324 :D

@alexander-akait
Copy link
Member

let's solve issue first, and when create monorepo 👍

@alexander-akait
Copy link
Member

I think problem inside css-loader, try to debug

@jquense
Copy link
Contributor Author

jquense commented Jul 18, 2019

This isn't fixed is is it. Looks like there was just a test case added

@alexander-akait
Copy link
Member

@jquense What output you expected in this case?

@jquense
Copy link
Contributor Author

jquense commented Jul 18, 2019

I noted it here:

#973 (comment)

The output should be .button\.hey so that it's interpreted as a single class. The default getLocalIdent uses cssesc which FIXES the locals export but not the css source.

@jquense
Copy link
Contributor Author

jquense commented Jul 18, 2019

i'll update the test case

@alexander-akait
Copy link
Member

@jquense in theory you can use cssesc on your line, other question should we do this by default

@alexander-akait
Copy link
Member

alexander-akait commented Jul 18, 2019

I see what you mean, maybe other developers specifically want to add a class, but i can't imagine use case

@jquense
Copy link
Contributor Author

jquense commented Jul 18, 2019

we do use cssesc in the default getLocalIdent, but it wasn't working for me locally. HOWEVER, while trying to update the test case and it DOES seem to escape everything. so you may be right its a problem with my css setup!

I think it makes sense to leave escaping up to the dev if they use getLocalIdent the problem when there are . in paths when using localIdentName to interpolate a name

@alexander-akait
Copy link
Member

@jquense yep, it is very rare use case, we can't change this right now only in next major release, now it is breaking change. Also developers really can want insert own output from getLocalIdent, but as i said above i can't imagine use case

@michaeltaranto
Copy link

michaeltaranto commented Jul 25, 2019

I think this might be the same issue I am facing. Since this commit this commit we have been unable to upgrade css-loader as . now end up in the selector name.

// css-loader options:
{
  module: 'local',
  localIdentName: '[name]__[local]___[hash:base64:7]'
}

// button.css.js
export default {
  '.container': { /* css rules */ }
}

The resulting classname previously was button-css__container__<hash>, since the change it is now button.css__container__<hash>.

Was the intention that cssesc would handle this escaping?

(Also if this is unrelated i can create a separate issue).

@alexander-akait
Copy link
Member

@michaeltaranto can you create minimum reproducible test repo?

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