Fix named export/import formatting#4609
Conversation
|
@saschanaz |
This pattern can also be used to format JSX end tag.
|
@DanielRosenwasser I just fixed it, sorry! ;) |
|
looks like we have a marge conflict with the other formatting changes. |
There was a problem hiding this comment.
I'm having trouble understanding why these were removed.
There was a problem hiding this comment.
@DanielRosenwasser The SpaceAfterAwaitKeyword and SpaceAfterTypeKeyword rules are merged into SpaceAfterCertainKeywords and SpaceAfterCertainTypeScriptKeywords rules respectively.
There was a problem hiding this comment.
What about the NoSpaceAfterAwaitKeyword one? Is that just encompassed by SpaceAfterCertainKeywords?
There was a problem hiding this comment.
The other NoSpace ones should not exist. I once thought they will work together with Space rules but then found they should only exist when there are some valid formatting options.
There was a problem hiding this comment.
I once thought NoSpace rules would first remove those whitespaces and after that Space rules would add one. However, a Space rule can remove excessive whitespaces by itself.
There was a problem hiding this comment.
I once thought
NoSpacerules would first remove those whitespaces and after thatSpacerules would add one. However, aSpacerule can remove excessive whitespaces by itself.
That's what I thought as well - it does make sense that one doesn't necessarily need the other though.
|
Looks good to me - @vladima can you take a quick peek? |
There was a problem hiding this comment.
Not sure why this is needed? Can we just do what is done for all other kinds of blocks, namely:
- add
SyntaxKind.NamedImports,SyntaxKind.NamedExportsto the list inshouldIndentChildNodefunction so its content will be indented - update
isCompletedNodefunction to correctly process case when cursor is in incompletedNamesImport/NamedExportnode by usinghasChildOfKind(n, SyntaxKind.CloseBraceToken, sourceFile);
There was a problem hiding this comment.
hasBraceShell function is not to check completeness as ImportClause node can omit its optional braces. I think isCompletedNode function is to check a node has its required children.
There was a problem hiding this comment.
@vladima I was wrong and the braces are not optional. I just removed hasBraceShell and added a simpler check.
There was a problem hiding this comment.
@saschanaz currently nodeIsCompleted handles cases when code is incomplete i.e.
import {$ // hit enter at $, caret on the next line should be indented
```.
I would expect imports and export to follow the same technique that we already use for all other blocks\block like constructionsThere was a problem hiding this comment.
Can you move this comment down? Also, is it correct?
There was a problem hiding this comment.
I later changed it to // NamedImports has its own braces as Block does (so do not give it additional indentation)
There was a problem hiding this comment.
@vladima Former hasBraceShell (and current namedBindings check) is needed to prevent unwanted excessive indentation. Consider this code:
import
{
x as xx
}
from "foo"
import
* as Foo from "foo"ImportDeclaration has ImportClause node and then ImportClause node can have two different kind of nodes: NamedImports and NamespaceImport.
NamedImports, as you noted, looks like a block, but NamespaceImport doesn't. If I decide to always indent ImportClause or always not to do it, formatting will look bad:
import
{
x as xx
} // when ImportClause is always indented
from "foo"
import
* as Foo from "foo" // when always notThus I added this check.
Anyway I agree that isCompletedNode should be updated. I'll push a change for it.
There was a problem hiding this comment.
we don't use nulls - use undefined instead
|
LGTM after comments are addresses |
|
looks good. please address the comments about use of null. |
|
I want to format export/imports and object destructuring (not in this PR) with similar rules but I'm not sure what should I do in this case: export
{ // Export block in this PR can optionally have a newline before open brace as `if` block can
x, y
} from "foo"
let
{ // Will anyone want this?
x, y
} = foo; |
|
👍 |
d8a8843 to
a9245c6
Compare
… fixExportImportFormatting
… fixExportImportFormatting
b8445ed to
a8ac0ef
Compare
|
thanks @saschanaz. can you also add a condition for #9224 to allow users to opt in/out. |
Fixes #4516, fixes #6924.