This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Correctly format JavaScript imports with long module paths
ClosedPublic

Authored by JaredNeil on Jun 30 2017, 6:01 PM.

Details

Summary

Currently the UnwrappedLineParser fails to correctly unwrap JavaScript imports where the module path is not on the same line as the from keyword.
For example:

import {A} from
'some/path/longer/than/column/limit/module.js';

This causes issues when in the middle a list of imports because the formatter thinks it has reached the end of the imports, and therefore will not sort any imports lower in the list.

The formatter will, however, split the from keyword and the module path if the path exceeds the column limit, which triggers the issue the next time the file is formatted.

Diff Detail

Repository
rL LLVM

Event Timeline

JaredNeil created this revision.Jun 30 2017, 6:01 PM
mprobst edited edge metadata.Jul 1 2017, 1:26 AM

Thanks for the contribution! Two nits.

unittests/Format/SortImportsTestJS.cpp
287 ↗(On Diff #104971)

Do you need all the other imports? Wouldn't a minimal test be to just have:

import {A} from
'y';
import {A} from 'x';

And then see that 'x' does get sorted before 'y'?

292 ↗(On Diff #104971)

Could you add a comment that this repro's a specific issue where we didn't parse the import correctly? It's non-obvious otherwise.

JaredNeil added inline comments.Jul 15 2017, 10:34 PM
unittests/Format/SortImportsTestJS.cpp
287 ↗(On Diff #104971)

Yeah, that does cover this specific issue.
I guess I put the other ones in there to make sure it parsed correctly no matter where the newline was.

Though after looking at it again, the tests really could be combined into a single test like:

import
{
A
}
from
'y';
import {A} from 'x';

Would that be better or worse than the simple one you suggested?

mprobst added inline comments.Jul 17 2017, 12:17 AM
unittests/Format/SortImportsTestJS.cpp
287 ↗(On Diff #104971)

I think that's roughly the same as what I suggested, isn't it? Looks good in any case.

Combined the sorting tests into a single test that covers all of those cases.

mprobst added inline comments.Jul 18 2017, 7:00 AM
unittests/Format/FormatTestJS.cpp
1459 ↗(On Diff #106900)

FYI I updated this to have some different whitespace in the original input, so that we can be sure something actually was changed and clang-format didn't bail somehow.

This revision was automatically updated to reflect the committed changes.