Update `getPos` type inside `NodeViewRendererProps` to reflect actual functionality

WilliamIPark commented 2 months ago

Changes Overview

The type for getPos inside of NodeViewRendererProps was incorrectly typed to indicate that it always returns a number, when this is not the actual case. It can also return undefined.

As per the ProseMirror documentation:

The third argument getPos is a function that can be called to get the node's current position, which can be useful when creating transactions to update it. Note that if the node is not in the document, the position returned by this function will be undefined.

Trusting this function to always return a number has lead to some stability issues in our application, so I believe it's best that the type reflects the actual functionality, even if this may cause some headaches to handle undefined cases.

Implementation Approach

Update the type to point towards Parameters<NodeViewConstructor>[2], which I believe inherits off pm/view.

Testing Done

Ran test suite, including cypress tests. All passing.

Verification Steps

Try to use getPos inside of a NodeView.

Additional Notes

I'd consider this a breaking change, as it could flare up people's type checks if they're not handling the possibility of the return value being undefined, so I have marked it as needing a major version bump in the changeset.

Checklist

[x] I have created a changeset for this PR if necessary.
[x] My changes do not break the library.
[x] I have added tests where applicable.
[x] I have followed the project guidelines.
[x] I have fixed any lint issues.

changeset-bot[bot] commented 2 months ago

🦋 Changeset detected

Latest commit: 8614cbb2d0b12e021bd4c755dc95e1e51e2f28b7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 54 packages

| Name | Type | | -------------------------------------- | ----- | | @tiptap/core | Major | | @tiptap/extension-blockquote | Major | | @tiptap/extension-bold | Major | | @tiptap/extension-bubble-menu | Major | | @tiptap/extension-bullet-list | Major | | @tiptap/extension-character-count | Major | | @tiptap/extension-code-block-lowlight | Major | | @tiptap/extension-code-block | Major | | @tiptap/extension-code | Major | | @tiptap/extension-collaboration-cursor | Major | | @tiptap/extension-collaboration | Major | | @tiptap/extension-color | Major | | @tiptap/extension-document | Major | | @tiptap/extension-dropcursor | Major | | @tiptap/extension-floating-menu | Major | | @tiptap/extension-focus | Major | | @tiptap/extension-font-family | Major | | @tiptap/extension-gapcursor | Major | | @tiptap/extension-hard-break | Major | | @tiptap/extension-heading | Major | | @tiptap/extension-highlight | Major | | @tiptap/extension-history | Major | | @tiptap/extension-horizontal-rule | Major | | @tiptap/extension-image | Major | | @tiptap/extension-italic | Major | | @tiptap/extension-link | Major | | @tiptap/extension-list-item | Major | | @tiptap/extension-list-keymap | Major | | @tiptap/extension-mention | Major | | @tiptap/extension-ordered-list | Major | | @tiptap/extension-paragraph | Major | | @tiptap/extension-placeholder | Major | | @tiptap/extension-strike | Major | | @tiptap/extension-subscript | Major | | @tiptap/extension-superscript | Major | | @tiptap/extension-table-cell | Major | | @tiptap/extension-table-header | Major | | @tiptap/extension-table-row | Major | | @tiptap/extension-table | Major | | @tiptap/extension-task-item | Major | | @tiptap/extension-task-list | Major | | @tiptap/extension-text-align | Major | | @tiptap/extension-text-style | Major | | @tiptap/extension-text | Major | | @tiptap/extension-typography | Major | | @tiptap/extension-underline | Major | | @tiptap/extension-youtube | Major | | @tiptap/html | Major | | @tiptap/react | Major | | @tiptap/starter-kit | Major | | @tiptap/suggestion | Major | | @tiptap/vue-2 | Major | | @tiptap/vue-3 | Major | | @tiptap/pm | Major |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

netlify[bot] commented 2 months ago

Deploy Preview for tiptap-embed ready!

Name	Link
Latest commit	8614cbb2d0b12e021bd4c755dc95e1e51e2f28b7
Latest deploy log	https://app.netlify.com/sites/tiptap-embed/deploys/66e9475f6ae18800081117ce
Deploy Preview	https://deploy-preview-5623--tiptap-embed.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

nperez0111 commented 2 months ago

As mentioned in the comment, this has to go towards a new major version so this PR is going to sit for a while

WilliamIPark commented 1 month ago

Yep sounds good :) Just wanted to make sure it wasn't left behind when that time comes!

ueberdosis / tiptap