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

Disable fixing files if fixing can cause bugs #4573

Merged
merged 3 commits into from
Feb 9, 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
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 eslint-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;

}

interface Input {
css?: string;
}
}
7 changes: 5 additions & 2 deletions types/stylelint/index.d.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
declare module 'stylelint' {
import {Result, ResultMessage, WarningOptions, Warning} from 'postcss';
import {Result, ResultMessage, WarningOptions, Warning, Root as PostcssRoot, NodeSource} from 'postcss';

export type StylelintConfigExtends = string | Array<string>;
export type StylelintConfigPlugins = string | Array<string>;
Expand Down 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