Autofix: jsx-indent: Add support for tabs

[jayphelps]

Everything seems correct except for the very last test.

The real output is:

function App() {
  return (
    <App>
  <Foo />
</App>
  );
}

But I would expect the output to be:

function App() {
  return (
    <App>
        <Foo />
    </App>
  );
}

Anyone have any ideas what I'm doing wrong or if somehow this is indeed expected behavior?

[jayphelps]

I would expect the output to be

function App() {
  return (
    <App>
        <Foo />
    </App>
  );
}

Why is it not?

[ljharb]

I agree - let's not merge this without the fix for this.

[jayphelps]

@ljharb a fix included in this PR or a fix elsewhere?

@CiGit had said this:

To have the output you (and I) expect, the way errors are computed should be changed. ie JSXElement's indentation compared to something like ROOT - 1line, not to its direct parent / opening tag.

IMO that's an other issue

If I understand correctly, this will require detection logic changes. I unfortunately don't have cycles to dig that deep into the internals at the moment. If anyone else does, feel free to continue--I don't need commit "credit" hehe

[ljharb]

I'd rather leave the broken but correct test, and comment it out, than leave this incorrect test. Could we do that?

[coveralls]

Coverage increased (+0.002%) to 96.771% when pulling f4d0ad4 on jayphelps:jsx-indent-fix into 82b3aa9 on yannickcr:master.

[CiGit]

In this case, only the <App> tag is an error so the fix function only applies to it.

Test case

To have the output you (and I) expect, the way errors are computed should be changed.
ie JSXElement's indentation compared to something like ROOT - 1line, not to its direct parent / opening tag.

IMO that's an other issue

[zouxuoz]

@jayphelps @CiGit How can I help to merge this PR?

[jayphelps]

Abandoning due to lack of response from maintainers.

---

Edit: I didn't mean this in a "fuck you guys for not responding" way. I PR'd this in May and hadn't received a response from any of the maintainers; and I don't have cycles to revisit in-depth at the moment. There are crazy number of perfectly valid reasons someone didn't respond, I'm not judging. Don't read into more than that 💃

[ljharb]

Sorry for the delay. Looks like this needs a rebase, plus a test change.

[jayphelps]

@ljharb oooo when I tried to rebase, I discovered it appears this has already been added in #830. Our two implementation seem mutually exclusive. Do you know if there is something this fixes that #830 did not?

[mmiller42]

@jayphelps I know of one -- I don't think #830 doesn't support tabs. That could be filed as a separate bug. (Currently it counts the number of characters in the indent and inserts spaces, I believe.)

[jayphelps]

@mmiller42 oh! my PR takes a different approach that does support that. Instead of trying to insert or remove indention, it just always just replaces the indention entirely using fixer.replaceTextRange.

I've rebased and updated my PR, uncommented the tests which weren't working in #830 but now are. Seems OK, but it's been a while since I've looked at this code so definitely have a critical eye.

[jayphelps]

Note that I have not fixed the pre-existing issue of the previously mentioned odd output in the test:

function App() {
  return (
    <App>
  <Foo />
</App>
  );
}

Presumably, #830 just didn't catch that.

[jayphelps]

This previous code used String.prototype.repeat() which is ES2015. I can switch back to using that instead of the join() trick I used, if it's a safe to assume repeat is supported.

[ljharb]

Alternatively you can just bring in a dep like https://www.npmjs.com/package/string-repeat

[jayphelps]

just let me know which way you'd like. to me it seems silly to bring in a lib since the join trick is short, simple, and I think fairly obvious what it does. 😏 aka left-pad

[ljharb]

In general, left-pad in no way changes that more deps is better. left-pad was only a problem for people that were already ignoring a decade of best practices and depending on third-party registries for their dependencies.

In this case, keeping your join trick is fine too.

[ljharb]

Thanks, this is a strict improvement on #830 and the added test cases are great!

I've got this one comment, plus potentially using a "repeat" package instead of the join trick, and then this should be good to go!

[jayphelps]

I'd rather leave the broken but correct test, and comment it out, than leave this incorrect test. Could we do that?

Done.

[jayphelps]

lmk if there's anything else I missed 🎉

[yannickcr]

Merged, thanks! And sorry for the lack of responses.

[sassanh]

Is it released to npm? How can we use it?

[yannickcr]

@sassanh yes, it's available in v6.5.0. You can use it with eslint --fix file.js (see ESLint documentation).

[sassanh]

That was fast. Thanks.