improve property access completion in markup #1381
Conversation
…nto editing-member-access
…nto editing-member-access
| @@ -5,7 +5,7 @@ if(hello){ | |||
| hello; | |||
| }} | |||
| { try { const $$_value = await (aPromise); { const foo = $$_value; | |||
| const hello = foo; | |||
| const hello = foo; | |||
There was a problem hiding this comment.
The existing test mostly is just whitespaces difference.
| * Usually it's there because of the preprocess we have before svelte parses | ||
| * the template | ||
| */ | ||
| export function getNodeEndIncludingTrailingPropertyAccess( |
There was a problem hiding this comment.
Should I import from the v1 utils? I found there's some utility copied from v1. Maybe the plan is to remove v1 eventually this way it's easier to remove?
There was a problem hiding this comment.
Yeah the plan is to remove v1 at some point, so it's okay to duplicate it here
| * Usually it's there because of the preprocess we have before svelte parses | ||
| * the template | ||
| */ | ||
| export function getNodeEndIncludingTrailingPropertyAccess( |
There was a problem hiding this comment.
Wondering if the name of this function is too long.
There was a problem hiding this comment.
What about getOriginalNodeEnd? Edit: After having written my other comment I think this would not be correct, since it only handles property access, not other cases. Maybe withTrailingPropertyAccess?
| @@ -115,6 +115,51 @@ function test(useNewTransformation: boolean) { | |||
| }); | |||
| }); | |||
|
|
|||
| const editingTestPositionsNewOnly: Array<[number, number, string]> = [ | |||
There was a problem hiding this comment.
These four doesn't work in v1. Wondering if it still makes sense to make it work.
There was a problem hiding this comment.
I wouldn't worry too much about the old transformation at this point, so I'm fine with it only working for the new transformation
| ): number { | ||
| let index = position; | ||
|
|
||
| while (index < originalText.length) { |
There was a problem hiding this comment.
Could we simplify this to originalText.indexOf('}', position) - 1 ? This is always used in a situation where there's a closing bracket of a mustache tag at the end. Or is this wrong because we only can do this for property assignments and not things like +, & etc which we also blank?
(having written this out, I think so, so I think it's fine as it is now)
There was a problem hiding this comment.
Yeah, it's currently only used for . and ?.. Not other operators we blanked. This condition for the while loop is mostly for preventing an infinite loop. Just so it's safer than while(true). In most cases, it would hit other characters and break the loop.
|
Looks good to me, left a couple of small comments. About the |
Related to #538, #786 and #1302
This PR added a utility to check if there's trailing property access behind an expression. The Reason we needed this is that we blank the operators before parsing. So the operator won't exist in the AST. We need to check it so that the expression transformation also include it.
This should only be called with an expression that is a direct child of a template node. And since we only go backwards from the end of the moustache (
}), it only makes sense to call this with the expression that might end directly before the end of the moustache.I also change the v2 transformation for
{@debug},{@const}and{@await}to have a better sourcemap. I can't find a way to make{@debug}and{@const}work with thetransformfunction. So I change it to directly overwrite.I have also experimented, but not committed yet, with skipping
)while blanking operators before parsing here:language-tools/packages/svelte2tsx/src/utils/htmlxparser.ts
Line 167 in cad755f
This would let the property access also work in a function call and the key of each block. Haven't thought of a downside for this. Tested and it works well and the existing test still passed. Wondering if I should do this in this PR or a separate one.