Properly handle multi-line tag expression replacements

[rwe]

trafficstars

Previously template literal replacemenets were made by constructing dummy strings of a particular length. Unfortunately if the inside of a tag has whitespace or multiple lines, this fails correctly position the error.

This adjusts the implementation to keep a mapping of source code locations to query document locations, and look up from that mapping to correlate errors.

Based on apollographql/eslint-plugin-graphql#122

TODO:

• [x] Make sure all of the significant new logic is covered by tests
• [x] Rebase your changes on master so that they can be merged easily
• [x] Make sure all tests pass
• [x] Update CHANGELOG.md with your change
• [x] If this was a change that affects the external API, update the README

Pull Request Labels

• [ ] feature
• [ ] blocking
• [ ] docs
• [x] bug

[jnwng]

closing #122 resulted in some merge conflicts that i don't think im in a position to resolve. do you mind resolving them?

[rwe]

Sure! Half a sec.

[rwe]

Rebased this and #144.

[rwe]

@jnwng — Any chance that this and/or #144 could be reviewed by anyone on the team? This one is not a huge headache, but #144 (which requires this) is very useful.

[jnwng]

Yes, I’ve been on vacation but can review upon return! On Tue, Jun 19, 2018 at 9:01 PM Robert Estelle [email protected] wrote:

@jnwng https://github.com/jnwng — Any chance that this and/or #144 https://github.com/apollographql/eslint-plugin-graphql/pull/144 could be reviewed by anyone on the team? This one is not a huge headache, but #144 https://github.com/apollographql/eslint-plugin-graphql/pull/144 (which requires this) is very useful.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/apollographql/eslint-plugin-graphql/pull/143#issuecomment-398509459, or mute the thread https://github.com/notifications/unsubscribe-auth/ABERRtO1RjV5wpscjW7BTluZQq4LimJ0ks5t-UqmgaJpZM4TrpKp .

[rwe]

Hi @jnwng, just a friendly bump if you might have some time to take a look at this and/or #144 🙂

[rwe]

@stubailo — I'd originally considered that approach but abandoned it as piling too many band-aids on top of each other. The placeholder-string approach doesn't work generally as a method of keeping the locations consistent.

Specifically, explicitly mapping query-text locations to source-code locations is required for proper location handling in #144, which adds support for spliced query literals from multiple source locations. (But correctly maps error messages to source locations).

This PR was mainly created as a simplified base from that, to show that the mapping approach behaves in the same way as before (modulo the mentioned bugfix). It would be great if you could please take a look at that PR to better understand the motivation for this one.

Even if this multiline template string fix were a specific aim, solving it with placeholders becomes more complex due to the variations of things like tracking "remaining characters in this line, leading characters in next line" and so-on.

Will rebase this and #144 once I have a chance since they just become conflicted, but please take another look!

[stubailo]

OK @erydo then can you please rename the PR to something more general than handling multi-line tag interpolations? I'm just thinking that I agree with what you're saying but I don't agree that it's the simplest way to solve the specific problem listed in the title.

[rwe]

Thanks @stubailo, will do. You're right the title buries the lede. I'll update it along with the rebase within next 12hrs and will ping back.