Implement a plugin that tracks node positions. #33
Conversation
The plugin provides a mechanism for node views to uniquely identify themselves via a key, as well as determine the position of other node views from their respective keys. Keys remain mostly stable across changes to the document. This will enable other improvements, like a safer mechanism for accessing node position from within NodeViews and proper React Context hierarchies across node view components.
There was a problem hiding this comment.
Looks great.
What do you think about following ProseMirror's naming convention for plugin factories a little bit more by dropping the Plugin suffix from the filenames and factory names?
In other words, just:
import { reactNodeViews } from 'react-prosemirror';
EditorState.create({ schema, plugins: [reactNodeViews(), collab()] })
|
I'm fine with the factory functions, but I'm a little worried about the idea of just exporting something called |
|
I'm curious whether the relatively close naming of |
|
It's kind of interesting... the |
|
I'm kinda okay with it being less descriptive, I think. Contextually, it'll make sense, and especially with API documentation and examples. Folks will import it and include it without thinking much of it, I think. |
|
Yeah, I guess we probably do not want to track state of the node views within the |
Taking that a little further, then: What if we just named it |
I feel good about that. Also, it feels like the key (:smirk:) thing that integrates React with ProseMirror is keying of nodes. That's what bridges the gap between React and ProseMirror reconciliation. |
|
Although, will that feel really awful if you import it in a file where you also import |
|
|
I think it won't? I feel like, since
I don't like this because explaining why it's called this requires explaining a bunch of internal details that consumers definitely do not need to know. So either it's just gibberish by design or it's way too in-the-weeds. |
|
I'm fine to try |
|
Now is the time to play with names and get feedback, after all! |
|
Done! |
smoores-dev
force-pushed
the
position-tracking-plugin
branch
from
dcca1ad
to
b3dbac9
Compare
|
Cleaned those up, thanks for spotting haha |
|
Do we need any README changes? |
The The |
| state: { | ||
| init(_, state) { | ||
| const next = { | ||
| posToKey: new Map<number, string>(), |
There was a problem hiding this comment.
Is there a reason that we are using a string on line 36 and NodeKey on the following line?
There was a problem hiding this comment.
Oh interesting, yeah this is worth thinking about a bit more, maybe. NodeKey is just "string + Symbol('portal rogistry root key')", and technically we never actually put that root symbol in the plugin's bimap. The reason that keyToPos is typed as having NodeKey is just so that consumers don't have to do a special-case check to see whether they have the root symbol or a string before trying to get the position from the keyToPos map (that is, it's type-safe to do pluginState.keyToPos.get(nodeKey), where nodeKey is of type NodeKey).
On the other hand, since we never actually put that root symbol in the bimap, technically it's correct that pluginState.posToKey(pos) always returns string, and you can use a string with any interface that accepts a NodeKey (since NodeKey is a subtype of string). So I think from a purely type-checking standpoint, this is "correct", but I also think it's somewhat unintuitive and might seem wrong to someone who's just using the API (which isn't actually exposed to users, so this would just be "us in the future", but we should avoid confusing "us in the future", too!).
The only downside I can think of to typing posToKey as Map<number, NodeKey> is that you can technically take any result of posToKey and pass it to any function that expects a string, but if it was typed as NodeKey, Typescript wouldn't let you do that! I dunno what functions you'd be passing a node key to that weren't typed as NodeKey, so this seems pretty minor, as downsides go (and it's an internal API so it's not a big deal if we have to change it later).
Any thoughts after all that? Haha
There was a problem hiding this comment.
Also we should probably change the Symbol name! That was from a previous version of this plugin, and it doesn't really make sense here
The plugin tracks a unique key for each (non-text) node in the document, identified by its current position. This provides a mechanism for node views to uniquely identify themselves via a key, as well as determine the position of other node views from their respective keys. Keys remain mostly stable across changes to the document.
This will enable other improvements, like a safer mechanism for accessing node position from within NodeViews and proper React Context hierarchies across node view components.