Fix incorrectly using 0-indexed number where 1-indexed number is expected

[u9g]

This pr causes console spam in our case for when we hover a comment on the first line. See obi1kenobi/trustfall#285 for more context.

What does this pr do?

Fixes an off-by-one bug that caused console spam when hovering things in the first line of a graphql file. This occurs because monaco's toGraphQLPosition() from monaco-graphql returns 0-based line/col numbers and getRange() from graphql-language-service expects 1-based line/col numbers.

Why is this the right fix?
We have a 0-indexed row/col. To see this for myself, I put a console.log() after this line. I then hovered the first character on the first line. I see 0,0 printed, and this is after we subtract 1 from both line/col in toGraphQLPosition(), so we definitely have a 0-indexed number.

getRange() takes in a SourceLocation, however that's an interface, so it's not really obvious whether the line/col is 0-based or 1-based. However, there is a function getLocation() that returns a SourceLocation declared in the same location.d.ts from within the graphql/language repo. So taking a look at their getLocation() function, it looks like this:

function getLocation(source, position) {
  let lastLineStart = 0;
  let line = 1;

  for (const match of source.body.matchAll(LineRegExp)) {
    typeof match.index === 'number' || (0, _invariant.invariant)(false);

    if (match.index >= position) {
      break;
    }

    lastLineStart = match.index + match[0].length;
    line += 1;
  }

  return {
    line,
    column: position + 1 - lastLineStart,
  };
}

When I saw that, it became clear that this line is 1-indexed, and the + 1 in the column leads me to believe this number is 1-based too. (if this isn't, please tell me!)

[changeset-bot]

🦋 Changeset detected

Latest commit: 1d988c1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
monaco-graphql	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: u9g (d9df970, fb0020c, 10cd0f7, 20b60d2, 1d988c1)
• ✅ login: TallTed / name: Ted Thibodeau Jr (10cd0f7, 1d988c1)

[acao]

@u9g thanks, this also fixes a bug in monaco-graphql iirc, can you add a changeset?

[u9g]

@u9g thanks, this also fixes a bug in monaco-graphql iirc, can you add a changeset?

I added a changeset for Fix incorrect loop condition, should I add a changeset for fix error range handling bug too if it only fixed bugs in the test as far as I know?

[acao]

huh. seeing the change you made to the language service makes me want to check for regressions in monaco-graphql and other places where we don't have tests yet

[acao]

from what i can tell, this is a breaking change, so we need to give the graphql-language-service a major bump

[acao]

yeah, looks from the e2e tests that youll need to handle the breaking change in codemirror-graphql as well

[u9g]

yeah, looks from the e2e tests that youll need to handle the breaking change in codemirror-graphql as well

I actually took another look at my though process and remade this pr, take a look at the pr description for why I made the changes.

[acao]

just a nit but this would be very useful to have in this file as toMonacoPosition() in ./utils ? I think this is the only place where we need to invoke it, but that could change. Aside from hover, the rest of the language services we currently implement and will implement tend to use ranges.

[TallTed]

to improve human clarity