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 https://github.com/obi1kenobi/trustfall/issues/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[bot]]

🦋 Changeset detected

Latest commit: 1d988c1f9345672978b4d8b26d5508e0fa516086

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[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: u9g (d9df97062ab721d5c74208c051bc3ba7be252a80, fb0020cfd286c0bdd294463c54df034a1c5a3883, 10cd0f73f67a5c31ddf8b9ab473f97ddcd8ed97c, 20b60d2a32db3b7a8b7a36d6da32348f4a98fed3, 1d988c1f9345672978b4d8b26d5508e0fa516086)
• :white_check_mark: login: TallTed / name: Ted Thibodeau Jr (10cd0f73f67a5c31ddf8b9ab473f97ddcd8ed97c, 1d988c1f9345672978b4d8b26d5508e0fa516086)

[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.