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

Reinstate and document workaround feature #4592

Merged
merged 9 commits into from
Feb 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 11 additions & 12 deletions docs/about/syntaxes.md
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
# Syntaxes

There are many styling languages, ranging from CSS language extension like SCSS to entirely different notations like CSS-in-JS objects. These styling languages can be then be embedded within other languages, including HTML `<style>` tags, markdown fences and JavaScript variables.
There are many styling languages, ranging from CSS language extensions like SCSS to entirely different notations, e.g. CSS-in-JS objects.

We aim to support all these use cases, but it's a complicated task.
These styling languages can be embedded within other languages too. For example:

We use [PostCSS syntaxes](https://github.com/postcss/postcss#syntaxes) to transform these languages into something that resembles CSS, which is the language that:
- HTML `<style>` tags
- markdown fences
- JavaScript template literals

- underpins all the other styling languages
- is best understood by stylelint
We aim to support all these use cases in stylelint, but it's a complicated endeavor.

We lean on [PostCSS syntaxes](https://github.com/postcss/postcss#syntaxes) to help us with this task. We use them to transform these languages into something that resembles CSS, which is the language that:

We need your help to [support and improve](https://github.com/postcss/postcss/blob/master/docs/syntax.md) the following PostCSS syntaxes:
- underpins all the other styling languages
- is best understood by rules built into stylelint

- [postcss-css-in-js](https://github.com/stylelint/postcss-css-in-js)
- [postcss-html](https://github.com/gucong3000/postcss-html)
- [postcss-less](https://github.com/webschik/postcss-less)
- [postcss-markdown](https://github.com/stylelint/postcss-markdown)
- [postcss-sass](https://github.com/AleshaOleg/postcss-sass)
- [postcss-scss](https://github.com/postcss/postcss-scss)
If you write your styles in anything other than CSS, please consider [contributing to these syntaxes](../developer-guide/syntaxes.md) so that they can remain compatible with stylelint.
35 changes: 35 additions & 0 deletions docs/developer-guide/syntaxes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# Working on syntaxes

Please help us enhance and debug the [syntaxes](../about/syntaxes.md) we use in stylelint:

- [postcss-css-in-js](https://github.com/stylelint/postcss-css-in-js)
- [postcss-html](https://github.com/gucong3000/postcss-html)
- [postcss-less](https://github.com/webschik/postcss-less)
- [postcss-markdown](https://github.com/stylelint/postcss-markdown)
- [postcss-sass](https://github.com/AleshaOleg/postcss-sass)
- [postcss-scss](https://github.com/postcss/postcss-scss)

To contribute to a syntax, you should:

1. Familiarize yourself with PostCSS's [how to write custom syntax](https://github.com/postcss/postcss/blob/master/docs/syntax.md) guide.
2. Use the [`syntax: *` labels](https://github.com/stylelint/stylelint/labels?utf8=%E2%9C%93&q=syntax%3A) to identify which syntax is behind an issue.
3. Go to the repository for that syntax.
4. Read their contributing guidelines.

## Workarounds

Fixing bugs in syntaxes can take time. stylelint can work around these bug by turning off autofix for incompatible sources. Autofix can then remain safe to use while contributors try to fix the underlying issue.

### Current workarounds

stylelint currently turns off autofix for sources that contain:

- nested tagged template literals ([issue #4119](https://github.com/stylelint/stylelint/issues/4119))

### Add a workaround

To add a new workaround, you should:

1. Add code to [`lib/lintSource.js`](https://github.com/stylelint/stylelint/blob/master/lib/lintSource.js) to detect the incompatible pattern.
2. Add a corresponding test to [`system-tests/fix/fix.test.js`](https://github.com/stylelint/stylelint/blob/master/system-tests/fix/fix.test.js).
3. Document the workaround in [`docs/developer-guides/syntaxes.md`](https://github.com/stylelint/stylelint/blob/master/docs/developer-guide/syntaxes.md).
1 change: 1 addition & 0 deletions docs/toc.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
- [Errors](user-guide/errors.md)
- Developer guide
- [Rules](developer-guide/rules.md)
- [Syntaxes](developer-guide/syntaxes.md)
- [Plugins](developer-guide/plugins.md)
- [Formatters](developer-guide/formatters.md)
- [System tests](developer-guide/system-tests.md)
Expand Down
2 changes: 1 addition & 1 deletion docs/user-guide/usage/options.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ Automatically fix, where possible, violations reported by rules.

For CSS with standard syntax, stylelint uses [postcss-safe-parser](https://github.com/postcss/postcss-safe-parser) to fix syntax errors.

**Note:** This is an _experimental_ feature. It currently does not respect special comments for disabling stylelint within sources (e. g. `/* stylelint-disable */`). Autofixing is applied regardless of these comments.
_`fix` is an experimental feature that currently ignores sources with [`/* stylelint-disable */` comments](../ignore-code.md)._

## `formatter`

Expand Down
63 changes: 62 additions & 1 deletion lib/lintSource.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,12 @@ function lintPostcssResult(stylelint, postcssResult, config) {
postcssResult.stylelint.ignoreDisables = true;
}

const isFileFixCompatible = isFixCompatible(postcssResult);

if (!isFileFixCompatible) {
postcssResult.stylelint.disableWritingFix = true;
}

const postcssRoots =
/** @type {import('postcss').Root[]} */ (postcssDoc &&
postcssDoc.constructor.name === 'Document'
Expand Down Expand Up @@ -200,7 +206,7 @@ function lintPostcssResult(stylelint, postcssResult, config) {
Promise.all(
postcssRoots.map((postcssRoot) =>
ruleFunction(primaryOption, secondaryOptions, {
fix: stylelint._options.fix,
fix: stylelint._options.fix && isFileFixCompatible,
newline,
})(postcssRoot, postcssResult),
),
Expand Down Expand Up @@ -241,3 +247,58 @@ function createEmptyPostcssResult(filePath) {
warn: () => {},
};
}

/**
* There are currently some bugs in the autofixer of Stylelint.
* The autofixer does not yet adhere to stylelint-disable comments, so if there are disabled
* ranges we can not autofix this document. More info in issue #2643.
* Also, if this document is parsed with postcss-jsx and there are nested template
* literals, it will duplicate some code. More info in issue #4119.
*
* @param {PostcssResult} postcssResult
* @returns {boolean}
*/
function isFixCompatible({ stylelint, root }) {
// Check for issue #2643
if (stylelint.disabledRanges.all.length) return false;

// Check for issue #4119
if (root && root.source && root.source.lang === 'jsx' && root.nodes) {
// Gather all locations of template literals
/**
* @typedef TemplateLiteralLocation
* @type {object}
* @property {number} startLine - Start of the template literal.
* @property {number} endLine - End of the template literal
*/
/** @type {Array<TemplateLiteralLocation>} */
const templateLiteralLocations = [];

root.nodes.forEach((n) => {
if (n.source && n.source.start && n.source.input.css !== undefined) {
templateLiteralLocations.push({
startLine: n.source.start.line,
endLine: n.source.start.line + n.source.input.css.split('\n').length,
});
}
});

// Compare all different template literal locations with eachother
for (const location1 of templateLiteralLocations) {
for (const location2 of templateLiteralLocations) {
// Make sure it's not the same template literal
if (location1 !== location2) {
// The first location should be before or after the second location.
// If not, it's not compatible.
if (
!(location1.endLine < location2.startLine || location2.endLine < location1.startLine)
) {
return false;
}
}
}
}
}

return true;
}
12 changes: 9 additions & 3 deletions lib/standalone.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,13 @@ module.exports = function(options) {
const returnValue = prepareReturnValue([stylelintResult]);

if (options.fix && postcssResult && !postcssResult.stylelint.ignored) {
// If we're fixing, the output should be the fixed code
returnValue.output = postcssResult.root.toString(postcssResult.opts.syntax);
if (!postcssResult.stylelint.disableWritingFix) {
// If we're fixing, the output should be the fixed code
returnValue.output = postcssResult.root.toString(postcssResult.opts.syntax);
} else {
// If the writing of the fix is disabled, the input code is returned as-is
returnValue.output = code;
}
}

return returnValue;
Expand Down Expand Up @@ -247,7 +252,8 @@ module.exports = function(options) {
postcssResult.root &&
postcssResult.opts &&
!postcssResult.stylelint.ignored &&
options.fix
options.fix &&
!postcssResult.stylelint.disableWritingFix
) {
// @ts-ignore TODO TYPES toString accepts 0 arguments
const fixedCss = postcssResult.root.toString(postcssResult.opts.syntax);
Expand Down
52 changes: 52 additions & 0 deletions system-tests/fix/fix.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,58 @@ describe('fix', () => {
);
});
});

it("doesn't fix with stylelint-disable commands", () => {
const code = `
/* stylelint-disable */
a {
color: red;
}
`;

return stylelint
.lint({
code,
config: {
rules: {
indentation: 2,
},
},
fix: true,
})
.then((result) => {
expect(result.output).toBe(code);
});
});

it("doesn't fix with nested template literals", () => {
const code = `
import styled, { css } from 'styled-components';

const Component = styled.div\`
padding: 10px;
\${() => css\`
color: #b02d00;
\`}
\`;
`;

return stylelint
.lint({
code,
syntax: 'css-in-js',
config: {
rules: {
indentation: 2,
},
},
fix: true,
})
.then((result) => {
expect(result.errored).toBe(true);
expect(result.output).toBe(code);
});
});
});

describe('fix with BOM', () => {
Expand Down
12 changes: 12 additions & 0 deletions types/postcss/extensions.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import * as postcss from 'postcss';
declare module 'postcss' {

interface NodeSource {
lang?: string;

}
Copy link
Member

Choose a reason for hiding this comment

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

[note] This lang property seems an extension by postcss-syntax. I cannot find it in the PostCSS core repository.

https://github.com/gucong3000/postcss-syntax/blob/0bca5a408f12891a622fd6149d8f23d3bd98df86/syntax.js#L17

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you know what the best way of handling this is?

Copy link
Member

Choose a reason for hiding this comment

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

No, I'm not sure now. 😣
This solution seems best for us now.


interface Input {
css?: string;
}
Copy link
Member

Choose a reason for hiding this comment

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

[note] This css property is missing the official PostCSS type definitions:

https://github.com/postcss/postcss/blob/ee267d5f6406e9a9ca88be4191ffb6c8b648dc15/lib/input.es6#L25-L34

}
5 changes: 4 additions & 1 deletion types/stylelint/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,15 @@ declare module 'stylelint' {
disabledRanges: DisabledRangeObject,
ignored?: boolean,
ignoreDisables?: boolean,
stylelintError?: boolean
stylelintError?: boolean,
disableWritingFix?: boolean
};

type EmptyResult = {
root: {
nodes?: undefined,
source: {
lang?: undefined,
input: {
file?: string
}
Expand Down