From c4305d0c07072f94b169331186fcb2a858b6e424 Mon Sep 17 00:00:00 2001 From: Adam Wathan Date: Fri, 10 Nov 2017 12:18:20 -0500 Subject: [PATCH 1/6] Don't allow `@apply`ing classes that ever appear inside of an at-rule This is too complex to easily support; better to explicitly error for now vs. the current behavior which is just silently doing something other than you probably expect. --- __tests__/applyAtRule.test.js | 41 ++++++++++---------------- src/lib/substituteClassApplyAtRules.js | 28 ++++++++++++++++-- src/util/findMixin.js | 17 ----------- 3 files changed, 40 insertions(+), 46 deletions(-) delete mode 100644 src/util/findMixin.js diff --git a/__tests__/applyAtRule.test.js b/__tests__/applyAtRule.test.js index 90bc8e0cf..5c0e96b26 100644 --- a/__tests__/applyAtRule.test.js +++ b/__tests__/applyAtRule.test.js @@ -14,34 +14,23 @@ test("it copies a class's declarations into itself", () => { }) }) -test("it doesn't copy a media query definition into itself", () => { - const output = `.a { - color: red; - } +test('applying classes that are ever used in a media query is not supported', () => { + const input = ` + .a { + color: red; + } - @media (min-width: 300px) { - .a { color: blue; } - } + @media (min-width: 300px) { + .a { color: blue; } + } - .b { - color: red; - }` - - return run( - `.a { - color: red; - } - - @media (min-width: 300px) { - .a { color: blue; } - } - - .b { - @apply .a; - }` - ).then(result => { - expect(result.css).toEqual(output) - expect(result.warnings().length).toBe(0) + .b { + @apply .a; + } + ` + expect.assertions(1) + return run(input).catch(e => { + expect(e).toMatchObject({ name: 'CssSyntaxError' }) }) }) diff --git a/src/lib/substituteClassApplyAtRules.js b/src/lib/substituteClassApplyAtRules.js index 11aaf3f98..8ff2b97d1 100644 --- a/src/lib/substituteClassApplyAtRules.js +++ b/src/lib/substituteClassApplyAtRules.js @@ -1,6 +1,5 @@ import _ from 'lodash' import postcss from 'postcss' -import findMixin from '../util/findMixin' import escapeClassName from '../util/escapeClassName' function normalizeClassNames(classNames) { @@ -9,6 +8,29 @@ function normalizeClassNames(classNames) { }) } +function findMixin(css, mixin, onError) { + const matches = [] + + css.walkRules(rule => { + if (rule.selectors.includes(mixin)) { + if (rule.parent.type !== 'root') { + onError( + `\`@apply\` cannot be used with ${mixin} because ${mixin} is nested inside of an at-rule (@${rule + .parent.name}).` + ) + } + + matches.push(rule) + } + }) + + if (_.isEmpty(matches) && _.isFunction(onError)) { + onError(`No ${mixin} class found.`) + } + + return _.flatten(matches.map(match => match.clone().nodes)) +} + export default function() { return function(css) { css.walkRules(rule => { @@ -27,8 +49,8 @@ export default function() { }) const decls = _.flatMap(classes, mixin => { - return findMixin(css, mixin, () => { - throw atRule.error(`No ${mixin} class found.`) + return findMixin(css, mixin, message => { + throw atRule.error(message) }) }) diff --git a/src/util/findMixin.js b/src/util/findMixin.js deleted file mode 100644 index 1460ea812..000000000 --- a/src/util/findMixin.js +++ /dev/null @@ -1,17 +0,0 @@ -import _ from 'lodash' - -export default function findMixin(css, mixin, onError) { - const matches = [] - - css.walkRules(rule => { - if (rule.selectors.includes(mixin) && rule.parent.type === 'root') { - matches.push(rule) - } - }) - - if (_.isEmpty(matches) && _.isFunction(onError)) { - onError() - } - - return _.flatten(matches.map(match => match.clone().nodes)) -} From e65b2df5a8775390e1aa9b75738b79ab9d135aee Mon Sep 17 00:00:00 2001 From: Adam Wathan Date: Mon, 13 Nov 2017 11:28:31 -0500 Subject: [PATCH 2/6] Improve error messages --- src/lib/substituteClassApplyAtRules.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/lib/substituteClassApplyAtRules.js b/src/lib/substituteClassApplyAtRules.js index 8ff2b97d1..ed5971d45 100644 --- a/src/lib/substituteClassApplyAtRules.js +++ b/src/lib/substituteClassApplyAtRules.js @@ -15,8 +15,7 @@ function findMixin(css, mixin, onError) { if (rule.selectors.includes(mixin)) { if (rule.parent.type !== 'root') { onError( - `\`@apply\` cannot be used with ${mixin} because ${mixin} is nested inside of an at-rule (@${rule - .parent.name}).` + `\`@apply\` cannot be used with ${mixin} because ${mixin} is nested inside of an at-rule (@${rule.parent.name}).` ) } @@ -25,7 +24,7 @@ function findMixin(css, mixin, onError) { }) if (_.isEmpty(matches) && _.isFunction(onError)) { - onError(`No ${mixin} class found.`) + onError(`\`@apply\` cannot be used with ${mixin} because ${mixin} either does not exist, or it's actual definition includes a pseudo-class like :hover, :active, etc.`) } return _.flatten(matches.map(match => match.clone().nodes)) From 6807e45e1eb507184d1da64f54684e26f6e5cf0c Mon Sep 17 00:00:00 2001 From: Adam Wathan Date: Thu, 16 Nov 2017 07:51:56 -0500 Subject: [PATCH 3/6] Expect onError function is always provided This can't really be optional. --- src/lib/substituteClassApplyAtRules.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/substituteClassApplyAtRules.js b/src/lib/substituteClassApplyAtRules.js index ed5971d45..4bb3eb09a 100644 --- a/src/lib/substituteClassApplyAtRules.js +++ b/src/lib/substituteClassApplyAtRules.js @@ -23,7 +23,7 @@ function findMixin(css, mixin, onError) { } }) - if (_.isEmpty(matches) && _.isFunction(onError)) { + if (_.isEmpty(matches)) { onError(`\`@apply\` cannot be used with ${mixin} because ${mixin} either does not exist, or it's actual definition includes a pseudo-class like :hover, :active, etc.`) } From afee4495d520d34dbed9f6177b6ee3001aa7a7cc Mon Sep 17 00:00:00 2001 From: Adam Wathan Date: Thu, 16 Nov 2017 07:52:30 -0500 Subject: [PATCH 4/6] Add test to document that applying classes with pseudo-selectors is not supported --- __tests__/applyAtRule.test.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/__tests__/applyAtRule.test.js b/__tests__/applyAtRule.test.js index 5c0e96b26..1bd33e0c7 100644 --- a/__tests__/applyAtRule.test.js +++ b/__tests__/applyAtRule.test.js @@ -37,5 +37,21 @@ test('applying classes that are ever used in a media query is not supported', () test('it fails if the class does not exist', () => { run('.b { @apply .a; }').catch(error => { expect(error.reason).toEqual('No .a class found.') +test('it does not match classes that include pseudo-selectors', () => { + const input = ` + .a:hover { + color: red; + } + + .b { + @apply .a; + } + ` + expect.assertions(1) + return run(input).catch(e => { + expect(e).toMatchObject({ name: 'CssSyntaxError' }) + }) +}) + }) }) From 538a854a739ee49ceaa8157cd4421a48c41852d1 Mon Sep 17 00:00:00 2001 From: Adam Wathan Date: Thu, 16 Nov 2017 07:54:40 -0500 Subject: [PATCH 5/6] Don't allow applying classes that appear in multiple rulesets This can result in unexpected behavior, so explicitly erroring is best. We can of course add support for this later if we see real value in it and can come up with predictable rules for how it should work. --- __tests__/applyAtRule.test.js | 26 +++++++++++++++++++++++--- src/lib/substituteClassApplyAtRules.js | 4 ++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/__tests__/applyAtRule.test.js b/__tests__/applyAtRule.test.js index 1bd33e0c7..98a47b9d2 100644 --- a/__tests__/applyAtRule.test.js +++ b/__tests__/applyAtRule.test.js @@ -14,6 +14,12 @@ test("it copies a class's declarations into itself", () => { }) }) +test('it fails if the class does not exist', () => { + return run('.b { @apply .a; }').catch(e => { + expect(e).toMatchObject({ name: 'CssSyntaxError' }) + }) +}) + test('applying classes that are ever used in a media query is not supported', () => { const input = ` .a { @@ -34,9 +40,6 @@ test('applying classes that are ever used in a media query is not supported', () }) }) -test('it fails if the class does not exist', () => { - run('.b { @apply .a; }').catch(error => { - expect(error.reason).toEqual('No .a class found.') test('it does not match classes that include pseudo-selectors', () => { const input = ` .a:hover { @@ -53,5 +56,22 @@ test('it does not match classes that include pseudo-selectors', () => { }) }) +test('it does not match classes that have multiple rules', () => { + const input = ` + .a { + color: red; + } + + .b { + @apply .a; + } + + .a { + color: blue; + } + ` + expect.assertions(1) + return run(input).catch(e => { + expect(e).toMatchObject({ name: 'CssSyntaxError' }) }) }) diff --git a/src/lib/substituteClassApplyAtRules.js b/src/lib/substituteClassApplyAtRules.js index 4bb3eb09a..b4101aa60 100644 --- a/src/lib/substituteClassApplyAtRules.js +++ b/src/lib/substituteClassApplyAtRules.js @@ -27,6 +27,10 @@ function findMixin(css, mixin, onError) { onError(`\`@apply\` cannot be used with ${mixin} because ${mixin} either does not exist, or it's actual definition includes a pseudo-class like :hover, :active, etc.`) } + if (matches.length > 1) { + onError(`\`@apply\` cannot be used with ${mixin} because ${mixin} is included in multiple rulesets.`) + } + return _.flatten(matches.map(match => match.clone().nodes)) } From 447bc873d8a9e4320ea845245da205e09518951f Mon Sep 17 00:00:00 2001 From: Adam Wathan Date: Thu, 16 Nov 2017 08:16:59 -0500 Subject: [PATCH 6/6] Prettier-ignore long error strings --- src/lib/substituteClassApplyAtRules.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/lib/substituteClassApplyAtRules.js b/src/lib/substituteClassApplyAtRules.js index b4101aa60..5408e82f6 100644 --- a/src/lib/substituteClassApplyAtRules.js +++ b/src/lib/substituteClassApplyAtRules.js @@ -14,9 +14,8 @@ function findMixin(css, mixin, onError) { css.walkRules(rule => { if (rule.selectors.includes(mixin)) { if (rule.parent.type !== 'root') { - onError( - `\`@apply\` cannot be used with ${mixin} because ${mixin} is nested inside of an at-rule (@${rule.parent.name}).` - ) + // prettier-ignore + onError(`\`@apply\` cannot be used with ${mixin} because ${mixin} is nested inside of an at-rule (@${rule.parent.name}).`) } matches.push(rule) @@ -24,10 +23,12 @@ function findMixin(css, mixin, onError) { }) if (_.isEmpty(matches)) { + // prettier-ignore onError(`\`@apply\` cannot be used with ${mixin} because ${mixin} either does not exist, or it's actual definition includes a pseudo-class like :hover, :active, etc.`) } if (matches.length > 1) { + // prettier-ignore onError(`\`@apply\` cannot be used with ${mixin} because ${mixin} is included in multiple rulesets.`) }