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

fix: keep scope when using schema.preceedingPlugins #2455

Merged
merged 6 commits into from
Apr 2, 2017

Conversation

joscha
Copy link
Contributor

@joscha joscha commented Mar 30, 2017

Which issue, if any, is this issue related to?

When creating a ruleTester, a schema can be passed in the test setup, e.g.:

const postCssModuleValues = require('postcss-modules-values');
{
    preceedingPlugins: [postCssModuleValues],
    ruleName: cssImportsOrder.ruleName,
    config: { preset: 'suit' },
    skipBasicChecks: true,
    accept: [...]
    reject: [...]
}

when using preceedingPlugins, this currently fails with:

   Uncaught TypeError: Cannot read property 'plugins' of undefined
      at use (node_modules/postcss/lib/processor.js:86:24)
      at Array.forEach (native)
      at postcssProcess (node_modules/stylelint/lib/testUtils/createRuleTester.js:163:32)
      at passingTestCases.forEach.acceptedCase (node_modules/stylelint/lib/testUtils/createRuleTester.js:179:29)
      at Array.forEach (native)
      at processGroup (node_modules/stylelint/lib/testUtils/createRuleTester.js:174:22)
      at process.nextTick (node_modules/stylelint/lib/testUtils/createRuleTester.js:117:9)
      at _combinedTickCallback (internal/process/next_tick.js:67:7)
      at process._tickCallback (internal/process/next_tick.js:98:9)
      at run (bootstrap_node.js:422:7)
      at startup (bootstrap_node.js:143:9)
      at bootstrap_node.js:537:3

due to the fact that the .use method of the postcss processor needs access to this.

Is there anything in the PR that needs further explanation?

Should be fairly straightforward.

@@ -160,7 +160,7 @@ function processGroup(rule, schema, equalityCheck) {
processor.use(assignDisabledRanges)

if (schema.preceedingPlugins) {
schema.preceedingPlugins.forEach(processor.use)
schema.preceedingPlugins.forEach(processor.use.bind(processor))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can also make this a schema.preceedingPlugins.forEach(plugin => processor.use(plugin)) if that's favorable

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do that: it fits the code conventions better.

@@ -160,7 +160,7 @@ function processGroup(rule, schema, equalityCheck) {
processor.use(assignDisabledRanges)

if (schema.preceedingPlugins) {
schema.preceedingPlugins.forEach(processor.use)
schema.preceedingPlugins.forEach(processor.use.bind(processor))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do that: it fits the code conventions better.

const createRuleTester = require("../createRuleTester")

describe("createRuleTester", () => {
it("is possible to create a tester", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great idea to add tests like this! My one reservation is that the one test covers several different possibilities. It was possible to create a tester before, right? — just not with preceedingPlugins? Maybe that means there should be a couple of separate tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can split them up. I just seem to have been the first that ever introduced a test for createRuleTester and the guidelines said to provide a test that exemplifies the issue ;) Happy to add another one before, they are quite verbose however as it pretty much uses the whole stylelint suite, but I agree it would be good to have them.

@joscha
Copy link
Contributor Author

joscha commented Mar 30, 2017

@davidtheclark changes are in 34f092d, please have another look

@joscha
Copy link
Contributor Author

joscha commented Apr 2, 2017

@davidtheclark any chance to get this fix in?

Copy link
Contributor

@davidtheclark davidtheclark left a comment

Choose a reason for hiding this comment

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

👍

@jeddy3 jeddy3 merged commit 1812744 into stylelint:master Apr 2, 2017
@jeddy3
Copy link
Member

jeddy3 commented Apr 2, 2017

Changelog:

  • Fixed: scope is kept when using schema.preceedingPlugins (#2455).

@joscha joscha deleted the patch-1 branch April 2, 2017 08:21
@joscha
Copy link
Contributor Author

joscha commented Apr 2, 2017

awesome, thank you @jeddy3 - any chance to get this in dot-release?

@jeddy3
Copy link
Member

jeddy3 commented Apr 2, 2017

any chance to get this in dot-release?

Sure thing. Patch releases don't take much time.

Released in 7.10.1.

@joscha
Copy link
Contributor Author

joscha commented Apr 2, 2017 via email

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

Successfully merging this pull request may close these issues.

None yet

3 participants