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] jsx-indent: Does not check indents for JSXText #2542

Merged
merged 1 commit into from Feb 1, 2020

Conversation

@toshi-toma
Copy link
Contributor

@toshi-toma toshi-toma commented Jan 14, 2020

Fixes #2467, Fixes #2484, Fixes #1136
This PR fixes the problem of does not check jsx-indent for JSXText.

JSXText Node is different from other formats.
e.g. The following code. JSXText's node.value returns \n text\n text\n .

const f = () => (
    <div>
        text
        text
    </div>
);
changes
  1. add handler for JSXText Node

    JSXText(node) {
    if (!node.parent) {
    return;
    }
    const parentNodeIndent = getNodeIndent(node.parent);
    checkJSXTextNodeIndent(node, parentNodeIndent + indentSize);
    }

  2. check JSXText Indent

    function matchAll(str, regex) {
    const matches = [];
    str.replace(regex, (...args) => {
    /** @type {object} */
    const match = Array.prototype.slice.call(args, 0, -2);
    match.input = args[args.length - 1];
    match.index = args[args.length - 2];
    matches.push(match);
    return '';
    });
    return matches;
    }
    /**
    * Check indent for JSXText
    * @param {ASTNode} node The node to check
    * @param {Number} indent needed indent
    */
    function checkJSXTextNodeIndent(node, indent) {
    const value = node.value;
    const regExp = indentType === 'space' ? /\n( *)[\t ]*\S/g : /\n(\t*)[\t ]*\S/g;
    const nodeIndentsPerLine = [];
    const matches = matchAll(value, regExp);
    matches.forEach((match) => {
    if (match[1]) {
    nodeIndentsPerLine.push(match[1].length);
    } else {
    nodeIndentsPerLine.push(0);
    }
    });
    const hasFirstInLineNode = nodeIndentsPerLine.length > 0;
    if (
    hasFirstInLineNode &&
    !nodeIndentsPerLine.every(actualIndent => actualIndent === indent)
    ) {
    report(node, indent, nodeIndentsPerLine);
    }
    }

  3. add the fix for JSXText

    if (node.type === 'JSXText') {
    const regExp = /\n[\t ]*(\S)/g;
    const fixedText = node.raw.replace(regExp, (match, p1) => `\n${indent}${p1}`);
    return fixer.replaceText(node, fixedText);
    }

  4. add test cases
    tests/lib/rules/jsx-indent.js

Copy link
Collaborator

@ljharb ljharb left a comment

Thanks, this looks great!

* @param {RegExp} regex
* @returns {Array}
*/
function matchAll(str, regex) {

This comment has been minimized.

@ljharb

ljharb Jan 16, 2020
Collaborator

there's no need to hardcode this when https://npmjs.com/string.prototype.matchall exists; please use that instead.

lib/rules/jsx-indent.js Outdated Show resolved Hide resolved
const matches = matchAll(value, regExp);
matches.forEach((match) => {
if (match[1]) {
nodeIndentsPerLine.push(match[1].length);

This comment has been minimized.

@ljharb

ljharb Jan 16, 2020
Collaborator

this whole body can then be replaced with:

return match[1] ? match[1].length : 0;
@ljharb ljharb force-pushed the toshi-toma:jsx-indent-for-jsxtext branch from 0076cb0 to 0264f6a Jan 16, 2020
@toshi-toma
Copy link
Contributor Author

@toshi-toma toshi-toma commented Jan 16, 2020

@ljharb
Thank you for the review!!
I fixed.

but, CI is failed...
The cause is JSXText only available Node to ESLint v5 or more.
eslint/eslint#10152

What do you think about supporting the ESLint version?

@ljharb ljharb force-pushed the toshi-toma:jsx-indent-for-jsxtext branch from eebc0bd to 137abcd Jan 28, 2020
@ljharb
Copy link
Collaborator

@ljharb ljharb commented Jan 28, 2020

@toshi-toma is there no way to identify text inside JSX in eslint < 5? It seems pretty likely that there is something, it's just got a different node type.

@golopot
Copy link
Contributor

@golopot golopot commented Jan 31, 2020

In older eslint (and current babel-eslint), the node type of text inside JSX is Literal.

tests/lib/rules/jsx-indent.js Show resolved Hide resolved
@toshi-toma
Copy link
Contributor Author

@toshi-toma toshi-toma commented Feb 1, 2020

@ljharb
I fixed to work for older eslint. and fixed some tests.
Now, CI is failing in Node v4...
I can't understand the cause.

lib/rules/jsx-indent.js Outdated Show resolved Hide resolved
@ljharb ljharb force-pushed the toshi-toma:jsx-indent-for-jsxtext branch from f13e0df to 8b89f61 Feb 1, 2020
@ljharb
ljharb approved these changes Feb 1, 2020
@ljharb ljharb force-pushed the toshi-toma:jsx-indent-for-jsxtext branch from 8b89f61 to ffdf69a Feb 1, 2020
@ljharb ljharb merged commit ffdf69a into yannickcr:master Feb 1, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 97.581%
Details
@toshi-toma toshi-toma deleted the toshi-toma:jsx-indent-for-jsxtext branch Feb 2, 2020
@ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 2, 2020

This caused #2561, which was fixed in 3da6eb3 - namely, the issue was that the literal check assumed strings.

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