fix: allow values in selectors to work correctly #894

jquense · 2019-02-15T18:54:08Z

This PR contains a:

Motivation / Use-Case

depends on:

fix: values should be replaced in selectors as well css-modules/postcss-modules-values#2 (comment)
fix: don't rescope imported names css-modules/postcss-modules-scope#4

This works to enable behavior that is specced by css-modules, but has been only partially implemented until now; TL; DR; selectors are a valid place for imported values to be interpolated.

Breaking Changes

Maybe! Still thinking through that

Additional Info

jsf-clabot · 2019-02-15T18:54:09Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

alexander-akait

Looks good. But:

Need tests.
Some developers can have same selector and @value test, it is potential breaking change.

alexander-akait · 2019-02-15T18:57:26Z

And it is not bugfix, it is feature, we never support @values in selectors

jquense · 2019-02-15T18:57:30Z

yup i'm thinking through the breaking change case, Might make this all opt-in initially...

Will definitely add tests

jquense · 2019-02-15T18:58:42Z

I would call it a bugfix since the behavior is not supposed to be new, it's specced out by css-modules. That said, it's never worked and it adds new behavior so if you want to call it a feature i understand

alexander-akait · 2019-02-15T19:02:11Z

@jquense Let's do this step by step. Please clarify what each PR do:

And how it is related to this PR

alexander-akait · 2019-02-15T19:04:58Z

src/plugins/postcss-icss-parser.js


-          if (typeof importIndex === 'number') {
+          if (replacement != null) {


Why not if (replacement) {}?

it could be! The old want needed to check for 0 but that's not the case anymore so lets do that!

src/plugins/postcss-icss-parser.js

alexander-akait · 2019-02-15T19:59:11Z

Need fix CI problem with naming commit, please read CONTRIBUTING.md

alexander-akait · 2019-02-15T19:59:21Z

Also please accept CLA

jquense · 2019-02-15T20:08:00Z

right so there are a few blocks to supporting this:

PR: css-modules/postcss-modules-values#2

This updates the values plugin so that it also replaces values in selectors. That behavior was always intended but it was using an old version of the icss utils that didn't support it.

PR: css-modules/postcss-modules-scope#4

Now that selectors are being replaced we run into a problem further down the chain in the scope plugin.

With the values PR this:

@value btn-class from './button.module.css'

.toolbar > .btn-class {
  margin-left: 10px
}

gets turned into:

:import('./button.module.css') {
  1__const_btn-class-0: btn-class
}

.toolbar > .1__const_btn-class-0 {
  margin-left: 10px
}

So that's what we want, the value of btn-class to be used as the class name here. Unfortunately the scope plugin with the default locals plugin will turn that selector into:

:local(.toolbar) > :local(.1__const_btn-class-0) {
 ...
}

This leads to the value identifier (1__const_btn) being hashed turning it into something like foo_module_12353-1__const_btn, which we don't want, because it removes the usage of the import.

This PR

The reason we need the last step is b/c after that pipeline we need to replace instances of import identifiers with a webpack require() to the file and the identifier there. Previously webpack wasn't checking in selectors for instances of local alias's so we needed to fix that.

jquense · 2019-02-15T20:09:51Z

Now that i'm thinking about it, it may be better to change the local by default plugin instead of the scope one...will check that

alexander-akait · 2019-02-15T20:11:48Z

Okey, we need major release for this feature, it is normal, but we need more tests here and in plugins

jquense · 2019-02-15T20:12:33Z

yes I will add the tests, I wanted to make sure this wasn't going to be rejected out of hand before doing all that :)

The lint issue doesn't seem to be because of this. I've got the commit named correctly already i believe

alexander-akait · 2019-02-15T20:16:03Z

@jquense just update lock file in other PR: rm -r node_modules && npm -r package-lock.json && npm install and send a new PR with new lock file, when we rebase this PR (security problems in some deps)

alexander-akait · 2019-02-18T10:18:15Z

/cc @jquense PR looks good, but we have some big problems in some plugins and i think we should solve this problems, need solve #578 as patch release and when merge other PRs as major release.

Problem:
css-selector-tokenizer break some selector (you can see this in first post in issue), we should migrate on postcss-selector-parser because he has better compatibility. Plugins which should be migrate on postcss-selector-parser:

Can you help with this?

jquense · 2019-02-18T13:40:47Z

sounds good i can work on that yeah 👍

alexander-akait · 2019-02-18T13:59:47Z

@jquense feel free to ping me

alexander-akait · 2019-03-07T10:12:47Z

/cc @jquense friendly ping, now we can merge and release new css modules plugins version and release major version

jquense · 2019-03-07T14:08:23Z

ok updated the other PR, need that merged and released before I can easily write tests here!

alexander-akait · 2019-03-11T12:08:40Z

/cc @jquense need rebase and fix CI, also can you provide link what should be merged first/second/etc, thanks

alexander-akait · 2019-03-25T16:53:12Z

/cc @jquense friendly ping

jquense · 2019-03-26T03:29:05Z

Sorry, didn't realize I was the bottleneck. Everything is up to date on my end. Waiting on css-modules/postcss-modules-local-by-default#13 to be merged, should have been more clear, realized I just linked the issue

alexander-akait · 2019-03-26T09:03:37Z

@jquense Steps:

jquense · 2019-03-26T16:33:04Z

only css-modules/postcss-modules-local-by-default#13
then this. I closed the other PR the local-by-default PR replaces it

alexander-akait · 2019-05-06T10:02:20Z

/cc @jquense sorry for very big delay, there are a lot of jobs, can you rebase and we release new version

alexander-akait · 2019-05-06T17:26:19Z

@jquense Just clarify, css-modules/postcss-modules-values#2 need merge too?

jquense · 2019-05-06T17:32:44Z

@evilebottnawi

these both need to be merged first:

fix: values should be replaced in selectors as well css-modules/postcss-modules-values#2
don't localize imported values in selectors css-modules/postcss-modules-local-by-default#13

README.md

jquense · 2019-05-13T12:51:10Z

@evilebottnawi this should be ready now 👍

alexander-akait

Great job, thanks!

alexander-akait · 2019-05-13T12:58:07Z

/cc @jquense i think we should rewrite https://github.com/css-modules/icss-utils/blob/master/src/replaceValueSymbols.js and use this in our code instead own replaceImportsInString, can you take care about rewriting this in icss-utils? And we refactor code in next PR

jquense · 2019-05-13T13:17:09Z

sure I can look into that

alexander-akait · 2019-05-13T13:19:50Z

@jquense thanks!

alexander-akait · 2019-05-15T17:05:27Z

Let's do release on this week

alexander-akait · 2019-05-28T18:47:10Z

/cc @jquense
Find interesting problem:

@value v-primary: #BF4040;
@value s-black: 'black-selector'; /* Problem here */
@value m-large: (min-width: 960px);

.primary-selector {
  color: v-primary;
}

.s-black {
  color: black;
}

@media m-large {
  .header {
    padding: 0 20px;
  }
}

I think we should fix it (or maybe better error message)

jquense · 2019-05-28T19:27:58Z

what's happening that seems wrong with the example?

alexander-akait · 2019-05-28T19:34:20Z

@jquense error when you compile due this line @value s-black: 'black-selector'; /* Problem here */ (quotes)

jquense · 2019-05-29T19:56:29Z

I don't think this is a new issue?

@value color: 'black';

.s-black {
  color: color;
}

is possible now and also won't work. Agree that a nicer error would be good but I don't think that's in scope for this PR?

alexander-akait · 2019-05-30T10:03:08Z

@jquense I already implement supporting @value in selector (in master), thanks for PR (my PR was inspired this PR), just find this error. Can you send other PR?

jquense · 2019-05-30T11:40:57Z

Ya you bet 👍 should we close this one then if you've pulled it in otherwise 😁

alexander-akait · 2019-05-30T11:49:43Z

@jquense i keep open issue due we should fix problem what i describe above

alexander-akait · 2019-06-04T17:57:15Z

/cc @jquense i can't do release before we can't fix problem with:

@value v-string: 'my-content';

.v-string {}

jquense · 2019-06-04T18:20:12Z

I'm sorry, i haven't had any time but i still don't understand why this is a blocker. The behavior of it not working is expected and consistent with the current v2 behavior for value interpolation.

jquense · 2019-06-04T18:21:33Z

Better, question what do you want it to do? having a selector with quotes is invalid, i don't think we should support that.

alexander-akait · 2019-06-04T18:22:04Z

Error like: Error: string (\"'my-content'\") is not allowed in a :local block\, not just throw error with stack

jquense · 2019-06-04T18:31:13Z

Unfortunately I don't think is no good place to check for and throw this error (unless you have an idea). It would likely involve a big refactor of the css-modules pipeline in order to even notice it since it's related to behavior across multiple postcss plugins.

I agree that the behavior isn't great, but it's not new behavior and it's LESS bad than other cases like this which fail silently. I don't think we should make it a release blocker.

Overall i'd be happy to work on these things generally but i don't have any access to the key repos or the authority to make larger structural changes necessary to fix these sort of systemic issues with css-modules. Besides yourself no one else is responsive on any repo either

alexander-akait · 2019-06-04T18:32:56Z

So we can't fix it in postcss-modules-local-by-default?

jquense · 2019-06-04T19:43:33Z

I don't think so...you'd only catch the ones for imported values, so your example above would still fail. We could try and put it in the values plugin but i'm not sure we know when the replacement is for a selector, since they all use the utils. I'd say we should put it in the utils module but that might be a larger scope than we'd want?

alexander-akait · 2019-06-04T20:41:46Z

Maybe we just throw error on String node in postcss-modules-local-by-default? I think it is not difficult

alexander-akait · 2019-06-04T22:43:36Z

Solved in postcss-modules-local-by-default

alexander-akait · 2019-06-04T22:43:54Z

I think tomorrow will be release new major version

jquense · 2019-06-05T02:48:17Z

awesome! glad you found a solution, I was thinking too hard about it 👍

alexander-akait reviewed Feb 15, 2019

View reviewed changes

src/plugins/postcss-icss-parser.js Show resolved Hide resolved

alexander-akait mentioned this pull request Feb 18, 2019

Lets try and consolidate repos, so it's easier to work on stuff!? css-modules/css-modules#312

Closed

jquense mentioned this pull request Mar 7, 2019

don't localize imported values in selectors css-modules/postcss-modules-local-by-default#13

Merged

alexander-akait reviewed May 8, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

docs: add media atRule example

e2ae777

jquense force-pushed the fix-selector-value-interpolation branch from 03807ba to e2ae777 Compare May 8, 2019 13:22

alexander-akait approved these changes May 13, 2019

View reviewed changes

alexander-akait closed this Jun 4, 2019

jquense deleted the fix-selector-value-interpolation branch June 5, 2019 02:47

jquense mentioned this pull request Sep 5, 2019

Referring to other Components does't work to Parcel? astroturfcss/astroturf#385

Open


		if (typeof importIndex === 'number') {
		if (replacement != null) {

fix: allow values in selectors to work correctly #894

fix: allow values in selectors to work correctly #894

Conversation

jquense commented Feb 15, 2019 • edited Loading

Motivation / Use-Case

Breaking Changes

Additional Info

jsf-clabot commented Feb 15, 2019 • edited Loading

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented Feb 15, 2019

jquense commented Feb 15, 2019

jquense commented Feb 15, 2019

alexander-akait commented Feb 15, 2019

alexander-akait Feb 15, 2019

Choose a reason for hiding this comment

jquense Feb 15, 2019

Choose a reason for hiding this comment

alexander-akait Feb 15, 2019

Choose a reason for hiding this comment

alexander-akait commented Feb 15, 2019

alexander-akait commented Feb 15, 2019

jquense commented Feb 15, 2019 • edited Loading

PR: css-modules/postcss-modules-values#2

PR: css-modules/postcss-modules-scope#4

This PR

jquense commented Feb 15, 2019

alexander-akait commented Feb 15, 2019

jquense commented Feb 15, 2019 • edited Loading

alexander-akait commented Feb 15, 2019

alexander-akait commented Feb 18, 2019 • edited Loading

jquense commented Feb 18, 2019

alexander-akait commented Feb 18, 2019

alexander-akait commented Mar 7, 2019

jquense commented Mar 7, 2019

alexander-akait commented Mar 11, 2019

alexander-akait commented Mar 25, 2019

jquense commented Mar 26, 2019

alexander-akait commented Mar 26, 2019 • edited Loading

jquense commented Mar 26, 2019

alexander-akait commented May 6, 2019

alexander-akait commented May 6, 2019

jquense commented May 6, 2019

jquense commented May 13, 2019

alexander-akait left a comment

Choose a reason for hiding this comment

alexander-akait commented May 13, 2019 • edited Loading

jquense commented May 13, 2019

alexander-akait commented May 13, 2019

alexander-akait commented May 15, 2019

alexander-akait commented May 28, 2019 • edited Loading

jquense commented May 28, 2019

alexander-akait commented May 28, 2019

jquense commented May 29, 2019

alexander-akait commented May 30, 2019 • edited Loading

jquense commented May 30, 2019

alexander-akait commented May 30, 2019

alexander-akait commented Jun 4, 2019

jquense commented Jun 4, 2019

jquense commented Jun 4, 2019

alexander-akait commented Jun 4, 2019 • edited Loading

jquense commented Jun 4, 2019

alexander-akait commented Jun 4, 2019

jquense commented Jun 4, 2019

alexander-akait commented Jun 4, 2019

alexander-akait commented Jun 4, 2019

alexander-akait commented Jun 4, 2019

jquense commented Jun 5, 2019

jquense commented Feb 15, 2019 •

edited

Loading

jsf-clabot commented Feb 15, 2019 •

edited

Loading

jquense commented Feb 15, 2019 •

edited

Loading

jquense commented Feb 15, 2019 •

edited

Loading

alexander-akait commented Feb 18, 2019 •

edited

Loading

alexander-akait commented Mar 26, 2019 •

edited

Loading

alexander-akait commented May 13, 2019 •

edited

Loading

alexander-akait commented May 28, 2019 •

edited

Loading

alexander-akait commented May 30, 2019 •

edited

Loading

alexander-akait commented Jun 4, 2019 •

edited

Loading