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.

Sign up for GitHub

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

Jump to bottom

Bugfix/separate selectors #1253

Merged
merged 4 commits into from Oct 14, 2019
Merged

Bugfix/separate selectors #1253

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
622e8a3
Fix #1196
fanich37 Oct 7, 2019
b73faa5
Add tests for grouping rules
fanich37 Oct 8, 2019
d3099c6
Use instance data instead of global const
fanich37 Oct 11, 2019
68d8718
Make solution more proper and easier to reason about
fanich37 Oct 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 32 additions & 5 deletions lib/selector.js
View file
Open in desktop
Expand Up @@ -3,10 +3,16 @@ let Prefixer = require('./prefixer')
let Browsers = require('./browsers')
let utils = require('./utils')

const SELECTORS_CACHE = {}

class Selector extends Prefixer {
constructor (name, prefixes, all) {
super(name, prefixes, all)
this.regexpCache = {}
SELECTORS_CACHE[name] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we replace it to this.selectors since we use only instance methods and data there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

possible: this.possible(),
replace: this.replace.bind(this)
}
}

/**
Expand Down Expand Up @@ -57,8 +63,29 @@ class Selector extends Prefixer {
}

let prefixeds = {}
for (let prefix of this.possible()) {
prefixeds[prefix] = this.replace(rule.selector, prefix)

if (rule.selector.includes(',')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let toProcess = rule.selector
.split(',')
.filter(el => el.includes(':'))
.map(el => el.trim())

Object.keys(SELECTORS_CACHE).forEach(name => {
let selector = toProcess.find(el => el.includes(name))
if (selector) {
prefixeds[name] = {}
SELECTORS_CACHE[name].possible.forEach(prefix => {
prefixeds[name][prefix] = SELECTORS_CACHE[name].replace(
selector, prefix)
})
}
})
} else {
prefixeds[this.name] = {}

for (let prefix of this.possible()) {
prefixeds[this.name][prefix] = this.replace(rule.selector, prefix)
}
}

rule._autoprefixerPrefixeds = prefixeds
Expand All @@ -79,8 +106,8 @@ class Selector extends Prefixer {
}

let some = false
for (let key in prefixeds) {
let prefixed = prefixeds[key]
for (let key in prefixeds[this.name]) {
let prefixed = prefixeds[this.name][key]
if (before.selector === prefixed) {
if (prefix === key) {
return true
Expand Down Expand Up @@ -117,7 +144,7 @@ class Selector extends Prefixer {
return
}

let cloned = this.clone(rule, { selector: prefixeds[prefix] })
let cloned = this.clone(rule, { selector: prefixeds[this.name][prefix] })
rule.parent.insertBefore(rule, cloned)
}

Expand Down
7 changes: 7 additions & 0 deletions test/autoprefixer.test.js
View file
Open in desktop
Expand Up @@ -44,6 +44,10 @@ let uncascader = autoprefixer({
let gradienter = autoprefixer({
overrideBrowserslist: ['Chrome 25', 'Opera 12', 'Android 2.3']
})
let grouping = autoprefixer({
overrideBrowserslist: ['Chrome 25', 'Firefox > 17', 'IE 10', 'Edge 12'],
grid: 'autoplace'
})
let ffgradienter = autoprefixer({
overrideBrowserslist: ['Chrome 25', 'Opera 12', 'Firefox 6']
})
Expand Down Expand Up @@ -90,6 +94,8 @@ function prefixer (name) {
return gradienter
} else if (name === 'gradient-fix') {
return ffgradienter
} else if (name === 'grouping-rule') {
return grouping
} else if (name === 'flexbox' || name === 'flex-rewrite') {
return flexboxer
} else if (name === 'double') {
Expand Down Expand Up @@ -244,6 +250,7 @@ it('saves declaration style', () => check('style'))
it('uses ignore next control comments', () => check('ignore-next'))
it('uses block control comments', () => check('disabled'))
it('has actual example in docs', () => check('example'))
it('process grouping rules correctly', () => check('grouping-rule'))

it('uses control comments to whole scope', () => {
let input = read('scope')
Expand Down
19 changes: 19 additions & 0 deletions test/cases/grouping-rule.css
View file
Open in desktop
@@ -0,0 +1,19 @@
.grid {
display: grid;
}

.a,
.b,
.c::selection,
.d:read-only,
.e::placeholder {
color: yellow;
}

::selection {
color: red;
}

:read-only {
color: black;
}
56 changes: 56 additions & 0 deletions test/cases/grouping-rule.out.css
View file
Open in desktop
@@ -0,0 +1,56 @@
.grid {
display: -ms-grid;
display: grid;
}

.c::-moz-selection {
color: yellow;
}

.e::-webkit-input-placeholder {
color: yellow;
}

.e:-moz-placeholder {
color: yellow;
}

.e::-moz-placeholder {
color: yellow;
}

.e:-ms-input-placeholder {
color: yellow;
}

.e::-ms-input-placeholder {
color: yellow;
}

.d:-moz-read-only {
color: yellow;
}

.a,
.b,
.c::selection,
.d:read-only,
.e::placeholder {
color: yellow;
}

::-moz-selection {
color: red;
}

::selection {
color: red;
}

:-moz-read-only {
color: black;
}

:read-only {
color: black;
}
33 changes: 27 additions & 6 deletions test/selector.test.js
View file
Open in desktop
Expand Up @@ -28,7 +28,7 @@ describe('regexp()', () => {
})

describe('check()', () => {
it('shecks rule selectors', () => {
it('checks rule selectors', () => {
let css = parse('body .selection {}, ' +
':::selection {}, body ::selection {}')
expect(selector.check(css.nodes[0])).toBeFalsy()
Expand All @@ -38,13 +38,34 @@ describe('check()', () => {
})

describe('prefixeds()', () => {
it('returns all avaiable prefixed selectors', () => {
it('returns all available prefixed selectors for grouping rule', () => {
let css = parse('.c::selection, .d:read-only {}')
let rSel = new Selector(':read-only', ['-moz-'])
expect(rSel.prefixeds(css.first)).toEqual({
'::selection': {
'-webkit-': '.c::-webkit-selection',
'-moz-': '.c::-moz-selection',
'-ms-': '.c::-ms-selection',
'-o-': '.c::-o-selection'
},
':read-only': {
'-webkit-': '.d:-webkit-read-only',
'-moz-': '.d:-moz-read-only',
'-ms-': '.d:-ms-read-only',
'-o-': '.d:-o-read-only'
}
})
})

it('returns all available prefixed selectors', () => {
let css = parse('::selection {}')
expect(selector.prefixeds(css.first)).toEqual({
'-webkit-': '::-webkit-selection',
'-moz-': '::-moz-selection',
'-ms-': '::-ms-selection',
'-o-': '::-o-selection'
'::selection': {
'-webkit-': '::-webkit-selection',
'-moz-': '::-moz-selection',
'-ms-': '::-ms-selection',
'-o-': '::-o-selection'
}
})
})
})
Expand Down