ProseMirror / prosemirror

The ProseMirror WYSIWYM editor
http://prosemirror.net/
MIT License
7.53k stars 334 forks source link

`ResolvedPos.node`, `.index` return type is incorrect #1463

Open rijenkii opened 2 months ago

rijenkii commented 2 months ago

This is a non-critical bug, as it mostly affects developer experience, but it is something that is bugging me a little.


https://github.com/ProseMirror/prosemirror-model/blob/4ea136e8aed30c3e47b7e2ffb3c9762318e0cb93/src/resolvedpos.ts#L47 https://github.com/ProseMirror/prosemirror-model/blob/4ea136e8aed30c3e47b7e2ffb3c9762318e0cb93/src/resolvedpos.ts#L52

Type of the return is not Node/number, but Node | undefined/number | undefined:

console.log($from.node(Infinity));
// undefined
console.log($from.index(Infinity));
// undefined

Infinity is used as an example of 100% invalid index value.


Because of this, the following errors are not captured by the type checker:

console.log($from.indexAfter(Infinity));
// NaN
console.log($from.start(Infinity));
// NaN

etc.

You can enable noUncheckedIndexedAccess in the tsconfig.json to catch the index-access-returning-unexpected-undefined errors, if you want to. This will break a lot of types, but only because your code does not currently expect the indexing operations to return undefined.

marijnh commented 2 months ago

The types assume that you pass in a valid depth. If you do that, you get a node. An out-of-bounds depth is considered a user error. Is this something you are running into in practice? Where are the depth values coming from?

rijenkii commented 2 months ago

Nothing critical, I'm just exploring and learning the lib.

It's just that I would expect a function that returns undefined to be annotated with return type that includes undefined. Otherwise this looks like an undocumented behaviour, as from the documentation alone I would expect an error to be thrown.

If your stance is solid, I'm fine with that.