Skip to content

[New] sort-prop-types: support comments on prop types #1973

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[New] sort-prop-types: support comments on prop types #1973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 3 additions & 20 deletions lib/rules/jsx-curly-spacing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
'use strict';

const has = require('has');
const commentsUtil = require('../util/comments');
const docsUrl = require('../util/docsUrl');

// ------------------------------------------------------------------------------
Expand Down Expand Up @@ -228,16 +229,7 @@ module.exports = {
message: `There should be no space after '${token.value}'`,
fix: function(fixer) {
const nextToken = sourceCode.getTokenAfter(token);
let nextComment;

// ESLint >=4.x
if (sourceCode.getCommentsAfter) {
nextComment = sourceCode.getCommentsAfter(token);
// ESLint 3.x
} else {
const potentialComment = sourceCode.getTokenAfter(token, {includeComments: true});
nextComment = nextToken === potentialComment ? [] : [potentialComment];
}
const nextComment = commentsUtil.getCommentsAfter(token, sourceCode);

// Take comments into consideration to narrow the fix range to what is actually affected. (See #1414)
if (nextComment.length > 0) {
Expand All @@ -262,16 +254,7 @@ module.exports = {
message: `There should be no space before '${token.value}'`,
fix: function(fixer) {
const previousToken = sourceCode.getTokenBefore(token);
let previousComment;

// ESLint >=4.x
if (sourceCode.getCommentsBefore) {
previousComment = sourceCode.getCommentsBefore(token);
// ESLint 3.x
} else {
const potentialComment = sourceCode.getTokenBefore(token, {includeComments: true});
previousComment = previousToken === potentialComment ? [] : [potentialComment];
}
const previousComment = commentsUtil.getCommentsBefore(token, sourceCode);

// Take comments into consideration to narrow the fix range to what is actually affected. (See #1414)
if (previousComment.length > 0) {
Expand Down
109 changes: 106 additions & 3 deletions lib/rules/sort-prop-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

const variableUtil = require('../util/variable');
const propsUtil = require('../util/props');
const commentsUtil = require('../util/comments');
const docsUrl = require('../util/docsUrl');
const propWrapperUtil = require('../util/propWrapper');

Expand Down Expand Up @@ -55,6 +56,7 @@ module.exports = {
const ignoreCase = configuration.ignoreCase || false;
const noSortAlphabetically = configuration.noSortAlphabetically || false;
const sortShapeProp = configuration.sortShapeProp || false;
const commentsAttachment = context.settings.comments || 'above';

function getKey(node) {
if (node.key && node.key.value) {
Expand Down Expand Up @@ -120,6 +122,90 @@ module.exports = {
return 0;
}

function getRelatedComments(node, nextNode) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this live in a shared utility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't mind that. Although I imagine the isSameLine bit in the output is pretty specific to this use case, I don't think it is generally useful to know whether the comment related to a thing is on the same line as the thing or not 🤔

// check for an end of line comment
const nextNodeComments = nextNode
? commentsUtil.getCommentsBefore(nextNode, sourceCode)
: commentsUtil.getCommentsAfter(node, sourceCode);
if (nextNodeComments.length === 1) {
const comment = nextNodeComments[0];
if (comment.loc.start.line === comment.loc.end.line && comment.loc.end.line === node.loc.end.line) {
return {comments: nextNodeComments, isSameLine: true};
}
}

if (commentsAttachment === 'above') {
return {comments: commentsUtil.getCommentsBefore(node, sourceCode), isSameLine: false};
}

return {comments: nextNodeComments, isSameLine: false};
}

function replaceNodeWithText(source, originalNode, sortedNodeText) {
return `${source.slice(0, originalNode.range[0])}${sortedNodeText}${source.slice(originalNode.range[1])}`;
}

function sortNodeWithComments(source, originalAttr, originalComments, sortedAttrText, sortedComments) {
if (sortedComments.length && originalComments.length) {
const swapComments = () => {
const sortedCommentsText = sourceCode.getText().slice(
sortedComments[0].range[0],
sortedComments[sortedComments.length - 1].range[1]
);
return `${source.slice(0, originalComments[0].range[0])}${sortedCommentsText}${source.slice(originalComments[originalComments.length - 1].range[1])}`;
};
if (originalAttr.range[1] < originalComments[0].range[0]) {
source = swapComments();
source = replaceNodeWithText(source, originalAttr, sortedAttrText);
} else {
source = replaceNodeWithText(source, originalAttr, sortedAttrText);
source = swapComments();
}
return source;
}

if (sortedComments.length) {
const sortedCommentsText = sourceCode.getText().slice(
sortedComments[0].range[0],
sortedComments[sortedComments.length - 1].range[1]
);

const indent = Array(sortedComments[0].loc.start.column + 1).join(' ');
if (commentsAttachment === 'above') {
source = replaceNodeWithText(source, originalAttr, sortedAttrText);
source = `${source.slice(0, originalAttr.range[0])}${sortedCommentsText}\n${indent}${source.slice(originalAttr.range[0])}`;
} else {
const nextToken = sourceCode.getTokenAfter(originalAttr);
const targetIndex = nextToken.value === ',' ? nextToken.range[1] : originalAttr.range[1];
source = `${source.slice(0, targetIndex)}\n${indent}${sortedCommentsText}${source.slice(targetIndex)}`;
source = replaceNodeWithText(source, originalAttr, sortedAttrText);
}
return source;
}

if (originalComments.length) {
const removeComments = () => {
const startLoc = sourceCode.getLocFromIndex(originalComments[0].range[0]);
const lineStart = sourceCode.getIndexFromLoc({line: startLoc.line, column: 0});
const endLoc = sourceCode.getLocFromIndex(originalComments[originalComments.length - 1].range[1]);
const lineEnd = sourceCode.getIndexFromLoc({
line: endLoc.line,
column: sourceCode.lines[endLoc.line - 1].length - 1
});
return `${source.slice(0, lineStart)}${source.slice(lineEnd + 2)}`;
};
if (originalAttr.range[1] < originalComments[0].range[0]) {
source = removeComments();
source = replaceNodeWithText(source, originalAttr, sortedAttrText);
} else {
source = replaceNodeWithText(source, originalAttr, sortedAttrText);
source = removeComments();
}
return source;
}

return null;
}

/**
* Checks if propTypes declarations are sorted
Expand Down Expand Up @@ -151,7 +237,16 @@ module.exports = {
for (let i = nodes.length - 1; i >= 0; i--) {
const sortedAttr = sortedAttributes[i];
const attr = nodes[i];
if (sortedAttr === attr) {
continue;
}

const sortedComments = getRelatedComments(sortedAttr,
allNodes[allNodes.indexOf(sortedAttr) + 1]).comments;
const attrComments = getRelatedComments(attr, nodes[i + 1]).comments;

let sortedAttrText = sourceCode.getText(sortedAttr);

if (sortShapeProp && isShapeProp(sortedAttr.value)) {
const shape = getShapeProperties(sortedAttr.value);
if (shape) {
Expand All @@ -162,16 +257,24 @@ module.exports = {
sortedAttrText = attrSource.slice(sortedAttr.range[0], sortedAttr.range[1]);
}
}
source = `${source.slice(0, attr.range[0])}${sortedAttrText}${source.slice(attr.range[1])}`;

const newSource = sortNodeWithComments(source, attr, attrComments, sortedAttrText, sortedComments);
source = newSource || replaceNodeWithText(source, attr, sortedAttrText);
}
});
return source;
}

const source = sortInSource(declarations, context.getSourceCode().getText());

const rangeStart = declarations[0].range[0];
const rangeEnd = declarations[declarations.length - 1].range[1];
const startComments = getRelatedComments(declarations[0], declarations[1]);
const endComments = getRelatedComments(declarations[declarations.length - 1], null);
const rangeStart = (commentsAttachment === 'above' && startComments.comments.length && !startComments.isSameLine)
? startComments.comments[0].range[0]
: declarations[0].range[0];
const rangeEnd = (commentsAttachment === 'below' && endComments.comments.length || endComments.isSameLine)
? endComments.comments[endComments.comments.length - 1].range[1]
: declarations[declarations.length - 1].range[1];
return fixer.replaceTextRange([rangeStart, rangeEnd], source.slice(rangeStart, rangeEnd));
}

Expand Down
76 changes: 76 additions & 0 deletions lib/util/comments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/**
* @fileoverview Utility functions for comments handling.
*/
'use strict';

/**
* Backports sourceCode.getCommentsBefore() for ESLint 3
*
* @param {Object} nodeOrToken Node or token to get comments for.
* @param {Object} sourceCode The SourceCode object.
* @returns {Array} Array of comment tokens.
*/
function getCommentsBefore(nodeOrToken, sourceCode) {
const token = sourceCode.getFirstToken(nodeOrToken, {includeComments: true});
let previousComments = [];

// ESLint >=4.x
if (sourceCode.getCommentsBefore) {
previousComments = sourceCode.getCommentsBefore(token);
// ESLint 3.x
} else {
let currentToken = token;
do {
const previousToken = sourceCode.getTokenBefore(currentToken);
Copy link
Contributor Author

@alexzherdev alexzherdev Sep 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Note: this code was extracted from jsx-curly-spacing and for some reason this line (or the next) returns undefined on ESLint 3.0.0 😕(our test suite on master fails against that version). Works well on 3.19.0.

Copy link
Member

Choose a reason for hiding this comment

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

Is this still failing? It'd be useful to figure out which version of 3.x this api was introduced in.

Copy link
Contributor Author

@alexzherdev alexzherdev Jan 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

So it was originally introduced in 3.0.0. My branch has 182 test failures on that version (not all of them seem to be related to this api or caused by my changes though. Master has 127 failures). Between 3.15.0 and 3.16.0 it drops from 169 to 34 failures, removing a lot of errors related to tokens/comments handling. Although a few of the remaining errors are me using getLocFromIndex in this PR, which was only introduced in 3.17.0 😕 Not sure what to do about that, it would not be feasible/doable to backport, but also there are other tests that are failing on that version anyway... We need at least 3.18.0 for 0 test failures.

Copy link
Member

Choose a reason for hiding this comment

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

If we backported the eslint API, would that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like both getLocFromIndex and getIndexFromLoc are relying heavily on lineStartIndices and lines in the SourceCode class that get populated once in the constructor. The ESLint docs explicitly say we can use lines (bottom of the section) but there's no word about lineStartIndices. Potentially we can just copy over those functions, but the bigger issue with getTokenBefore/getTokenAfter remains, and those don't seem possible to be backported.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a way we could runtime-detect these APIs, and provide a reasonable degraded experience for older eslint users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm skeptical (not that we could detect them, but that there is a reasonable experience beyond just not applying fixes), but let me look into that.

Copy link
Member

Choose a reason for hiding this comment

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

yeah that's what i meant - applying fewer fixes when they weren't good enough.

const potentialComment = sourceCode.getTokenBefore(currentToken, {includeComments: true});

if (previousToken !== potentialComment) {
previousComments.push(potentialComment);
currentToken = potentialComment;
} else {
currentToken = null;
}
} while (currentToken);
previousComments = previousComments.reverse();
}

return previousComments;
}

/**
* Backports sourceCode.getCommentsAfter() for ESLint 3
*
* @param {Object} nodeOrToken Node or token to get comments for.
* @param {Object} sourceCode The SourceCode object.
* @returns {Array} Array of comment tokens.
*/
function getCommentsAfter(nodeOrToken, sourceCode) {
const token = sourceCode.getLastToken(nodeOrToken, {includeComments: true});
let nextComments = [];

// ESLint >=4.x
if (sourceCode.getCommentsAfter) {
nextComments = sourceCode.getCommentsAfter(token);
// ESLint 3.x
} else {
let currentToken = token;
do {
const nextToken = sourceCode.getTokenAfter(currentToken);
const potentialComment = sourceCode.getTokenAfter(currentToken, {includeComments: true});

if (nextToken !== potentialComment) {
nextComments.push(potentialComment);
currentToken = potentialComment;
} else {
currentToken = null;
}
} while (currentToken);
}

return nextComments;
}

module.exports = {
getCommentsBefore: getCommentsBefore,
getCommentsAfter: getCommentsAfter
};
Loading