Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include node locations for HTML in connector-jsdom #1289

Closed
antross opened this issue Sep 5, 2018 · 7 comments
Closed

Include node locations for HTML in connector-jsdom #1289

antross opened this issue Sep 5, 2018 · 7 comments

Comments

@antross
Copy link
Member

antross commented Sep 5, 2018

This allows providing accurate line/col information for elements HTML even when other elements have been dynamically inserted/removed nearby. It is done by setting includeNodeLocations: true in the options passed to new JSDOM() and is already utilized in the new parser-html (PR #1277).

@molant
Copy link
Member

molant commented Sep 7, 2018

This depends on us updating to the latest API (#163), correct?

@antross
Copy link
Member Author

antross commented Sep 7, 2018

Yes, I don’t think this was an option on the old API (though I could be wrong).

@molant
Copy link
Member

molant commented Sep 7, 2018

@sarvaje can you add this to your PR #1277?

@sarvaje
Copy link
Contributor

sarvaje commented Sep 7, 2018

@sarvaje can you add this to your PR #1277?

That's not my PR :P. But yep, I will add it to my PR.

Yes, I don’t think this was an option on the old API (though I could be wrong).

The old API activate it by default.

@molant
Copy link
Member

molant commented Sep 7, 2018

The old API activate it by default.

Really? How did I miss that 😱
Maybe we should do some tests with various SPAs to see how the change in the location detection affects them?

@antross
Copy link
Member Author

antross commented Sep 7, 2018

I should note the location information doesn't yet flow all the way to the hints in production. I have a commit on the vscode extension PR that updates the IAsyncHTMLElement and JSDOMAsyncHTMLElement types to expose this: 1db7fa1

I can put this in a separate PR if we want.

@sarvaje
Copy link
Contributor

sarvaje commented Sep 7, 2018

Really? How did I miss that

Yep :)

I have added the option in the v12 PR #1274

@alrra alrra closed this as completed in f825d20 Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants