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 support for processors #3261

Merged
merged 2 commits into from May 16, 2018

Conversation

6 participants
@gucong3000
Member

gucong3000 commented Apr 13, 2018

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

Closes: #3252
Closes: styled-components/stylelint-processor-styled-components#180

Is there anything in the PR that needs further explanation?

No, it's self explanatory.

@gucong3000 gucong3000 referenced this pull request Apr 13, 2018

Closed

Why PostCSS #4

@gucong3000 gucong3000 force-pushed the issue-3248 branch from 39877e6 to 4b9637f Apr 14, 2018

@gucong3000 gucong3000 changed the title from [WIP] Reduce install size to [WIP] fix support for processors Apr 14, 2018

@gucong3000 gucong3000 force-pushed the issue-3248 branch 4 times, most recently from 0e172bf to 08895c4 Apr 14, 2018

@gucong3000 gucong3000 changed the title from [WIP] fix support for processors to Fix support for processors Apr 15, 2018

@jeddy3

I've requested some changes. I'll also detail a regression with the markdown processor in a comment below.

@@ -81,6 +81,11 @@
"svg-tags": "^1.0.0",
"table": "^4.0.1"
},
"optionalDependencies": {

This comment has been minimized.

@jeddy3

jeddy3 Apr 16, 2018

Member

According to the yarn docs:

Optional dependencies are just that: optional. If they fail to install, Yarn will still say the install process was successful.

This is useful for dependencies that won’t necessarily work on every machine and you have a fallback plan in case they are not installed (e.g. Watchman).

So, I don't think this is what want. Shall we move them to dependencies for now? At least it's explicit then.

(For anyone wondering, the postcss-markdown and postcss-styled dependencies are already included in 9.2 but they were hidden within postcss-html. So, removing them is a breaking change and a discussion for 10.0).

} else if (syntax) {
syntax = syntaxes[syntax];
if (!syntax) {
throw new Error(
"You must use a valid syntax option, either: scss, less or sugarss"
"You must use a valid syntax option, either: scss, sass, less, sugarss, markdown, styled or html"

This comment has been minimized.

@jeddy3

jeddy3 Apr 16, 2018

Member

Let's remove styled from this list for now as it's an undocumented feature. And let's remove markdown and html as we're still resolving the processor vs syntax issue.

We can discuss all this in #3252 (comment)

@@ -13,6 +13,11 @@ Processors are community packages that enable stylelint to lint the CSS within n
## PostCSS syntax

This comment has been minimized.

@jeddy3

jeddy3 Apr 16, 2018

Member

Let's just remove this whole section for now. This is the processors.md file. Once we resolve the relationship between processors and syntax we can revisit this.

@@ -17,10 +17,10 @@ By default, the linter can *parse* any the following non-standard syntaxes by us
- SCSS (using [`postcss-scss`](https://github.com/postcss/postcss-scss))
- Less (using [`postcss-less`](https://github.com/shellscape/postcss-less))
- SugarSS (using [`sugarss`](https://github.com/postcss/sugarss))
- HTML,

This comment has been minimized.

@jeddy3

jeddy3 Apr 16, 2018

Member

Let's remove "Javascript or typescript" from this list for now as it's an undocumented feature. And let's remove "Markdown" and "HTML-like" as we're still resolving the processor vs syntax issue.

This comment has been minimized.

@gucong3000
@jeddy3

This comment has been minimized.

Member

jeddy3 commented Apr 16, 2018

@gucong3000 Many thanks for starting on this!

Also, thanks for splitting the syntaxes into their own packages. I'm confident this will help down the line.

Sorry for the delay reviewing this PR. The changes are far reaching and there is a lot to understand.

I've tested this branch with all the processors listed in the docs:

The good news is most, including stylelint-processor-styled-components, are compatible with this branch. However, @mapbox/stylelint-processor-markdown is not.

Given the following config:

{
  "processors": ["@mapbox/stylelint-processor-markdown"],
  "extends": [
    "stylelint-config-standard"
  ]
}

And the following markdown:

Uh oh, I'm about to use SCSS.

```scss
// Parser-breaking comment
$foo: bar;
.foo {}
```

The following error is thrown:

jeddy3:stylelint-test jeddy3$ ./node_modules/.bin/stylelint "sources/md/scss.*" --config configs/markdown.json
TypeError: Cannot read property 'line' of undefined
    at result.warnings.reduce (/Users/jeddy3/Projects/stylelint-test/node_modules/@mapbox/stylelint-processor-markdown/index.js:54:39)
    at Array.reduce (<anonymous>)
    at transformResult (/Users/jeddy3/Projects/stylelint-test/node_modules/@mapbox/stylelint-processor-markdown/index.js:51:39)
    at config.resultProcessors.forEach.resultProcessor (/Users/jeddy3/Projects/stylelint/lib/createStylelintResult.js:174:26)
    at Array.forEach (<anonymous>)
    at stylelint.getConfigForFile.then (/Users/jeddy3/Projects/stylelint/lib/createStylelintResult.js:171:31)
    at <anonymous>

It is the scss (or less or sass) on the code fence that breaks it. Using css is compatible.

This regression in this branch will need to be fixed before we can merge this PR.

Then, after a patch is released, I believe stylelint will once again be compatible with the existing processors. We can then, as we prepare for stylelint@10.0, carefully deliberate on:

  1. the role of processors and syntax
  2. what features (e.g. markdown, <style>, styled components) stylelint should support by default.

@jeddy3 jeddy3 referenced this pull request Apr 16, 2018

Closed

Release 9.2.1 #3255

6 of 6 tasks complete

@gucong3000 gucong3000 force-pushed the issue-3248 branch from 08895c4 to ca37a82 Apr 18, 2018

@gucong3000

This comment has been minimized.

Member

gucong3000 commented Apr 18, 2018

However, @mapbox/stylelint-processor-markdown is not.

Still fail when I remove syntax from postcssOptions:
https://github.com/stylelint/stylelint/blob/issue-3248/lib/getPostcssResult.js#L92

Even remove postcss-syntax, is is still fail

@ntwb ntwb referenced this pull request Apr 28, 2018

Closed

Add CSS-in-JS support #3264

@jeddy3

This comment has been minimized.

Member

jeddy3 commented May 3, 2018

@gucong3000 Thanks for making the changes and for looking into the markdown processor regression. Sorry for the slow response. I'm just catching up on 2 weeks of stylelint issues.

Ok, I think we should merge this PR and get the release out. It's more important to fix the styled-components processor regression because users might be relying on the interpolation tagging feature of the processor. Anyone using the markdown-processor (with non-standard syntax code fences) can simply remove processor and I believe they'll get the same functionality. Sound good?

@jeddy3

jeddy3 approved these changes May 3, 2018

@gucong3000 gucong3000 force-pushed the issue-3248 branch 3 times, most recently from 8ba210a to 3588040 May 8, 2018

@stramel

This comment has been minimized.

stramel commented May 14, 2018

@jeddy3 any update on this?

@jeddy3

This comment has been minimized.

Member

jeddy3 commented May 14, 2018

It would be great to see this merged. @stylelint/core A second pair of eyes on it would be great, though.

If no one has time, I'll try to make some to test again this evening and get this merged.

@jeddy3 jeddy3 force-pushed the issue-3248 branch from 3588040 to faef5c8 May 16, 2018

@jeddy3

This comment has been minimized.

Member

jeddy3 commented May 16, 2018

I've rebased this on master and it tested it again locally. All looks good.

What I previously said is still valid:

  • this PR fixes the styled components and glamorous processors regressions (for users using interpolation tagging)
  • markdown and vue users can, for now, just remove any processors from their config and stylelint will work

Once this is out we can then deliberate over how best to combine processors and syntaxes for the most optimal user experience.

As such, I'd like to merge this PR and get a release out before I go on holiday this Friday. @stylelint/core Can someone give it second green tick please so I can merge?

@hudochenkov

This comment has been minimized.

Member

hudochenkov commented May 16, 2018

I'm not sure what this PR does in general. Does it introduce breaking change?

Code looks good.

"less",
"sass",
"scss",
"markdown",

This comment has been minimized.

@ntwb

ntwb May 16, 2018

Member

markdown was being removed wasn't it?

This comment has been minimized.

@gucong3000

gucong3000 May 16, 2018

Member

postcss-markdown split from the original postcss-html

"postcss-less": "^1.1.5",
"postcss-markdown": "^0.23.6",

This comment has been minimized.

@ntwb

ntwb May 16, 2018

Member

Likewise, wasn't markdown being removed for now, in this PR and to be added back in a PR of its own?

This comment has been minimized.

@gucong3000

gucong3000 May 16, 2018

Member

Previously Markdown's processing logic was written in postcss-html, and now it is split into multiple packages: postcss-syntax, postcss-html, postcss-styled and postcss-markdown

@@ -113,7 +115,7 @@
"precommit": "lint-staged",
"benchmark-rule": "node scripts/benchmark-rule.js",
"dry-release": "npmpub --dry --verbose",
"flow": "flow",
"flow": "echo flow",

This comment has been minimized.

@ntwb

ntwb May 16, 2018

Member

Why echo the flow word instead of running flow?

@gucong3000 gucong3000 force-pushed the issue-3248 branch from faef5c8 to ade70a5 May 16, 2018

@ntwb

ntwb approved these changes May 16, 2018

👍

@evilebottnawi

Great work!

@jeddy3

This comment has been minimized.

Member

jeddy3 commented May 16, 2018

I'm not sure what this PR does in general. Does it introduce breaking change?

@hudochenkov I don't believe there are any breaking changes. This PR fixes a regression with the styled components and glamorous processors. However, behind the scenes @gucong3000 has split postcss-html into different packages. This is helpful for us going forward. The majority of the changes in this PR update stylelint to use these packages whilst maintaining the existing documented functionality.

Once the dust has settled on this next patch release, we'll be in good position to decide if, and in what way, the exisiting functionality should change (e.g. should we continue to support markdown out the box?). It is these (later) decisions that could result in breaking changes.

@jeddy3 jeddy3 merged commit 9417497 into master May 16, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.003%) to 95.969%
Details

@jeddy3 jeddy3 deleted the issue-3248 branch May 16, 2018

@jeddy3

This comment has been minimized.

Member

jeddy3 commented May 16, 2018

  • Fixed: regression with processors (e.g. styled-components) (#3261).

bors bot added a commit to mozilla/delivery-console that referenced this pull request May 16, 2018

Merge #161
161: Update dependency stylelint to v9.2.1 r=rehandalal a=renovate[bot]

This Pull Request updates dependency [stylelint](https://github.com/stylelint/stylelint) from `v9.2.0` to `v9.2.1`



<details>
<summary>Release Notes</summary>

### [`v9.2.1`](https://github.com/stylelint/stylelint/blob/master/CHANGELOG.md#&#8203;921)
[Compare Source](stylelint/stylelint@9.2.0...0a7c7ed)
-   Fixed: `cache` option hiding CssSyntaxError outputs ([#&#8203;3258](`stylelint/stylelint#3258)).
-   Fixed: regression with processors (e.g. styled-components) ([#&#8203;3261](`stylelint/stylelint#3261)).
-   Fixed: `no-descending-specificity` false positives for Sass nested properties ([#&#8203;3283](`stylelint/stylelint#3283)).
-   Fixed: `selector-pseudo-class-no-unknown` false positives proprietary webkit pseudo classes when applied to a simple selector ([#&#8203;3271](`stylelint/stylelint#3271)).

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment