Bugfix/separate selectors #1253
Conversation
|
Wow, great. This fix is great (and looks like correct) but should have a double check (and I am too drunk right now). I will try to check it this week. |
lib/selector.js
Outdated
| class Selector extends Prefixer { | ||
| constructor (name, prefixes, all) { | ||
| super(name, prefixes, all) | ||
| this.regexpCache = {} | ||
| SELECTORS_CACHE[name] = { |
There was a problem hiding this comment.
How we will lean this cache? Will we have a problem if PostCSS will process many very different files (or even different versions of the same files) during the same Node.js session (so the cache will be alive?)
If you want a per-document cache, you can use node.root().autoprefixerSelectors cache. It will be attached only for the same CSS root.
There was a problem hiding this comment.
Oops, that's a really good question. I need to say I didn't even thought about such cases. I'll try to check them asap. Thanks!
There was a problem hiding this comment.
I haven't tested it yet. I just thought that SELECTORS_CACHE is filled with data during the file/files preprocessing. So probably it should work fine with multiple files. But I'm not 100% sure. By the way the naming SELECTORS_CACHE is kinda misleading. It's not a cache, it's just a collection.
There was a problem hiding this comment.
Can we replace it to this.selectors since we use only instance methods and data there?
There was a problem hiding this comment.
Nope, it wouldn't work in such implementation. The idea with this global const is to keep all possible selectors with all relevant prefixes. And with this.selectors we only get relevant selectors for this.name. But rule might contain several selectors.
I like your propose about using node.root().autoprefixerSelectors. I haven't checked it yet and actually I don't know how it works but may be It could help to get rid of SELECTORS_CACHE.
There was a problem hiding this comment.
I can try to do so. My concern is that replace method uses the instance method this.prefixed(prefix) inside. But hacks ::placeholder and :fullscreen are overriding it. The ::placeholder also overrides this.possible() method.
There was a problem hiding this comment.
My concern is that replace method uses the instance method this.prefixed(prefix) inside.
If it really rely on instance, you can't detouch it from this instance.
Don't forget that PostCSS can be called on many different CSS files.
There was a problem hiding this comment.
If it really rely on instance, you can't detouch it from this instance.
That's make sense.
I'll try to implement your idea.
Don't forget that PostCSS can be called on many different CSS files.
Can it process different files with different browser settings?
There was a problem hiding this comment.
Can it process different files with different browser settings?
Yeap. The same instance can be used for different target browsers or different whitespace settings.
It will be safer just to think that every PostCSS call must be independent from each other.
There was a problem hiding this comment.
I don't know what was I thinking when I decided to use global const SELECTORS_CACHE. Each instance of class Selector gets property all on its init. And all is just the Prefixes instance that already contains all the data we need to process grouping rules 😄
Thank for your fast reply! It would be awesome if you found some time to test it more precisely. But it's definitely not the fix to rush with 👍 |
lib/selector.js
Outdated
| @@ -1,3 +1,5 @@ | |||
| let list = require('postcss').list | |||
There was a problem hiding this comment.
| let list = require('postcss').list | |
| let { list } = require('postcss') |
There was a problem hiding this comment.
Replaced with your suggestion 👍
lib/selector.js
Outdated
| prefixeds[name] = {} | ||
|
|
||
| possible.forEach(prefix => { | ||
| if (selToProc.length > 1) { |
There was a problem hiding this comment.
I think we can remove this if. Array with a single element will have the same result after processing with map and join. It will be a little slower, but it is not a problem here since the main idea is to reduce code complexity.
| for (let prefix of this.possible()) { | ||
| prefixeds[prefix] = this.replace(rule.selector, prefix) | ||
|
|
||
| if (rule.selector.includes(',')) { |
There was a problem hiding this comment.
I completely lost the idea behind this method (but I wrote it 😭).
Can you describe (you can do it in Russian) what do we do here?
There was a problem hiding this comment.
I'll try to do it in english 🤞
This method collects all possible prefixes for the selector, creates prefixed version of the rule.selector, caches the result and adds it to the rule. So the rule looks like this at the end (copied from master branch):
Rule {
type: 'rule',
selector: '.a, .a .b::placeholder, a:read-only',
// lots of properties
_autoprefixerPrefixeds: {
'-webkit-': '.a, .a .b::-webkit-input-placeholder, a:read-only',
'-moz-': '.a, .a .b::-moz-placeholder, a:read-only',
'-ms-': '.a, .a .b::-ms-input-placeholder, a:read-only',
'-o-': '.a, .a .b::-o-placeholder, a:read-only',
'-moz- old': '.a, .a .b:-moz-placeholder, a:read-only',
'-ms- old': '.a, .a .b:-ms-input-placeholder, a:read-only' }
}
}As you can see :read-only is left without prefixes.
There was a problem hiding this comment.
I refactored code, and I think now it's much more easier to reason about. Plus, I decided that each selector has to process only that part of rule that contains the selector and not the whole rule as I did before.
|
Great! Now it became much simpler. |
fanich37
force-pushed
the
bugfix/separate_selectors
branch
from
820c3de
to
68d8718
Compare
|
Thanks. Released in 9.6.5. Great work in cleaning PR. |
Thanks for your review and questions that helped me to find the proper solution 👍 |
|
I have now an error with pseudo element |
Not sure about implementation at all but that's how I see it from my understanding of how the PostCSS and Autoprefixer works☺️