fix: report on correct location instead of entire quasi

[thepassle]

before:

after:

[thepassle]

Thinking about this more, I dont think the logic to 'select' the quasi is even needed anymore?

[thepassle]

Yep, seems like it's not needed any more. Checked with a few more test cases to make sure everything still works:

@43081j PTAL 🙂

[43081j]

i think there might be an edge case here with the column.

can you try:

render() {
  return html`<div attr="<">
    Contents
  </div>`;
}

as i imagine using endCol - 1 for every line except the first will work. since the first line's column would actually be (colOfFirstQuasi + endCol).

as in:

render() {
  return html`<div attr="<">
--------------^-- this is (14 + 0) i suppose
    Content
---^-- this is just 4, in both estree and parse5
  </div>`;
}

also why is it endCol - 1 and startCol - 1? is parse5's column 1-based and estree is 0-based?

[thepassle]

Good spot, looks like that does report incorrectly:

So only if the line is 1 we should append the col of the first quasi

is parse5's column 1-based and estree is 0-based?

Yep, thats what it seemed like judged on the values in the debugger and manually counting the lines, and watching the output in the IDE 😓

[43081j]

haha, i did just check the AST of both and yep, one is 0-based and the other is 1-based. consistent 😂

i also had another thought just now which is a little unfortunate... what if we have something like this:

render() {
  return html`
    Foo ${someVariable} Bar
  `;
}

The ${someVariable} will be a different length to the placeholder we internally add to the DOM.

So it may be the case that we do actually need to use the quasi's column rather than just the node's. not entirely sure off the top of my head how we'd solve that yet but you can recreate it probably by:

render() {
  return html`
    <div bar=${whatever} attr="<"></div>
  `;
}

because in reality the parse5 tree will be like:

<div bar="{{SOME_PLACEHOLDER}}" attr="<"></div>

the col of attr would differ

fortunately i think thats the only other edge case

[thepassle]

So it may be the case that we do actually need to use the quasi's column rather than just the node's. not entirely sure off the top of my head how we'd solve that yet but you can recreate it probably by:

Could this be fixed if we make the placeholder the same length as the expression? e.g.:

<div bar=${foobar}></div>

{{12345}}
${foobar}

[thepassle]

First edgecase is fixed:

Looking into the second one now

[thepassle]

Hrm, I got something working seemingly correctly for edgecase 2, but its not the prettiest code 🙈 Could you take a look @43081j ?

Here's a screenshot:

[thepassle]

Changing the placeholder will break the isExpressionPlaceholder check, but I dont see that used anywhere? 🤔

[43081j]

it would be nice to maintain a single placeholder. can we maybe use the quasi's line/col as offset? if we know that, we know the line and column the "string" begins at. we can locate the placeholder like in master and start from there maybe.

i can have a look tomorrow too if we're still stuck then

[thepassle]

Looks like more edgecases ☹️ Think I'll need some assistance after all 😓

correct:

but here it reports the error on the } after the .map

[thepassle]

I've updated the PR

[43081j]

i suspect this doesn't build or tests fail but travis has decided to go walkies...

ill see whats up with it

edit:

the build failures seem to be because an expression's location can be empty (if location info is forced off). ill leave some comments

[thepassle]

@43081j PTAL

[thepassle]

hrm looks like i broke a bunch of tests -.-

[thepassle]

okay, should be good to go now 👍