upgrade all the prosemirror libs to latest (incl. regenerating yarn.lock)
#350
Conversation
…ypes/prosemirror-view`
…arious prosemirror libs (and their interdependence)
…rosemirror-view` `1.30.2` - ProseMirror/prosemirror-view#153 - note the warning ProseMirror/prosemirror-view#153 (comment))
twrichards
force-pushed
the
upgrade/prosemirror-view
branch
from
24c8645
to
4c70100
Compare
| @@ -171,7 +171,7 @@ const createNodeView = < | |||
| ): NodeViewCreator => (initElementNode, view, _getPos, _, innerDecos) => { | |||
| const dom = document.createElement("div"); | |||
| dom.contentEditable = "false"; | |||
| const getPos = typeof _getPos === "boolean" ? () => 0 : _getPos; | |||
| const getPos = () => _getPos() ?? NaN; | |||
There was a problem hiding this comment.
Why is this change required?
There was a problem hiding this comment.
from PR description
notably to reflect type signature changes of
getPosfunction (introduced inprosemirror-view1.30.2- ProseMirror/prosemirror-view#153 - note the warning ProseMirror/prosemirror-view#153 (comment))
... the type signature of the getPos function passed in on line 171 (called _getPos) is now () => number | undefined which needs to be handled at some point as the getPos everywhere else in @guardian/prosemirror-elements expects () => number and the ripple effect of changing those type signatures then wasn't handled by the prosemirror libs that the functions are sometimes passed back into. So I changed this to return NaN if _getPos returns undefined so that it will explode in the relevant places whilst still adhering to the type number.
You're right to raise this though, as whatever was happening before would possibly return 0, which I didn't think was correct as the undefined is only returned if the node is not in the doc and returning 0 would imply its at the start of the doc... but perhaps there is logic littered around which relies on it falling back to zero rather than exploding. On balance, I think this change is worth it and nothing has exploded in my anecdotal testing thus far, let me know if you disagree though and I can perhaps have another go at propagating the () => number | undefined out further through the codebase.
There was a problem hiding this comment.
Sorry for missing that, too early in the morning.
When attempting to upgrade
prosemirror-viewfrom1.29.1to1.33.1(to take advantage of ProseMirror/prosemirror-view#165 and ProseMirror/prosemirror-view#166, as a pre-requisite to #349) I ran into some breaking changes to the types, notably to reflect type signature changes ofgetPosfunction (introduced inprosemirror-view1.30.2- ProseMirror/prosemirror-view#153 - note the warning ProseMirror/prosemirror-view#153 (comment)) but also cascaded into requiring upgrade ofprosemirror-state... so for good measure decided to upgrade all the prosemirror libs to latest.There were some very strange incompatibilities after I'd done this, which AFAICT derived from multiple conflicting versions of packages referenced in the
yarn.lockso I decided to delete and regenerate, which solved all the strange issues - but has resulted in a chunky diff - sorry.