Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update getPos type inside NodeViewRendererProps to reflect actual functionality #5623

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

WilliamIPark
Copy link
Contributor

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

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

Related Issues

#3823

Copy link

changeset-bot bot commented Sep 17, 2024

🦋 Changeset detected

Latest commit: 8614cbb

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

Copy link

netlify bot commented Sep 17, 2024

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 8614cbb
🔍 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...

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@nperez0111 nperez0111 changed the base branch from develop to next September 24, 2024 20:39
Copy link
Contributor

@nperez0111 nperez0111 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this into next branch

@nperez0111 nperez0111 merged commit 37913d5 into ueberdosis:next Sep 24, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants