vfile / vfile-location

utility to convert between positional (line and column-based) and offset (range-based) locations
https://unifiedjs.com
MIT License
13 stars 2 forks source link

Return undefined from `toPoint` if no match is found #10

Closed remcohaszing closed 1 year ago

remcohaszing commented 1 year ago

Initial checklist

Problem

I just updated this package from version 4.0.1 to 4.1.0. This broke type checking because of https://github.com/vfile/vfile-location/commit/3c377f1c4f7ea0115d07e695d35f80dee163c415. Now the result of toPoint() is no longer assignable to a unist Point. This commit makes the types technically accurate, which is a good thing. However, this highlights another issue.

As a user, I would expect toPoint to return a Point. This is also what’s documented in the readme. However, what’s really returned is, either a Point, or an object that ressembles a point, but has all values set to undefined.

This issue is similar to, but not the same as https://github.com/vfile/vfile-message/issues/15.

I understand that input may be out of bounds, and this needs to be handled. Typically this is handled by returning null / undefined. For example:

Some APIs throw instead:

Solution

Return undefined if no point is found.

This typically means usage will look something like this:

const { toPoint } = location(vfile)
const point = toPoint(offset)

if (!point) {
  return
}

// Use a unist point here

Or using a TypeScript non-null assertion if you know the offset is correct:

const { toPoint } = location(vfile)
const point = toPoint(offset)!

// Use a unist point here

Compared to the current usage:

const { toPoint } = location(vfile)
const point = toPoint(offset)

if (point.line === undefined || point.column === undefined || point.offset === undefined) {
  return
}

// Use a unist point here

Alternatives

  1. Return null instead of undefined. It doesn’t matter that much compared to undefined. This is consistent with some of the APIs I mentioned above, but lately I got the feeling null is being frowned upon a bit in the community.
  2. Throw an error. Although I think typically user’s don’t want to deal with try/catch logic.
wooorm commented 1 year ago

Yep, agreed!

github-actions[bot] commented 1 year ago

Hi! This was closed. Team: If this was fixed, please add phase/solved. Otherwise, please add one of the no/* labels.