ProseMirror / prosemirror

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

`GapCursor.findFrom` incompatibly overrides `Selection.findFrom` #1301

Open pvande opened 2 years ago

pvande commented 2 years ago

While upgrading my ProseMirror libraries, I was greeted with type errors in a small section of code:

    // Ensure that the selection is moved to the position immediately following the
    // just inserted Node.
    const insertionPos = txn.mapping.map(initialPos)
    const $found = GapCursor.findFrom(txn.doc.resolve(insertionPos), +1)
    if ($found) {
      txn.setSelection(new GapCursor($found))  // <-- Type Error
    } else if (txn.selection.jsonID === 'node') {
      const $pos = GapCursor.findFrom(txn.selection.$from, +1) || txn.selection.$to
      txn.setSelection(new GapCursor($pos))  // <-- Type Error
    }

Specifically, the error I received noted that GapCursor.findFrom should, per Selection.findFrom, return Selection | null, while the GapCursor constructor requires a ResolvedPos.

Digging into this a little more deeply, it seems that GapCursor doesn't actually document findFrom, but aliases this public method to the (internal) findGapCursorFrom, which returns a ResolvedPos.

marijnh commented 2 years ago

That static method doesn't appear to be part of the public interface, so you can't really expect anything from it.

pvande commented 2 years ago

On the one hand, I agree — GapCursor.findGapCursorFrom is explicitly marked @internal, and is therefore at best an unreliable interface.

On the other hand, Selection.findFrom is a documented member of the public interface, inherited by all Selection subclasses. Furthermore, GapCursor.findFrom is assigned, but never used within the prosemirror-gapcursor library (though I suspect that this was intended as a nod towards backwards compatibility after the TS refactor).