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

fix(vue-3): Correctly set editor's appContext.provide to forward full inject chain #5397

Conversation

romansp
Copy link
Contributor

@romansp romansp commented Jul 26, 2024

Fixes #5394.

Changes Overview

Replace usage of Object.assign and set appContext.provides as-is from current instance provides.

Implementation Approach

Vue internally uses prototype chain to preserve injects across the entire component chain. Thus should avoid Object.assign or spread operator as it won't copy the prototype. All correct provides will be already present on instance.provides.

Testing Done

Added new example with cypress test coverage.

Verification Steps

Additional Notes

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

@romansp romansp requested review from bdbch and svenadlung as code owners July 26, 2024 18:52
Copy link

changeset-bot bot commented Jul 26, 2024

🦋 Changeset detected

Latest commit: a71fca4

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

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

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 Jul 26, 2024

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit a71fca4
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/66a41fbf6480fe0008be6424
😎 Deploy Preview https://deploy-preview-5397--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.

@romansp
Copy link
Contributor Author

romansp commented Jul 26, 2024

Each example has associated tests

context('/src/Examples/Savvy/Vue/', () => {

but I don't see an existing one that would be "natural" for this. Maybe just a new folder in Examples/Integrations/Vue that copies one of the other instances maybe?

Let's continue discussion in this PR @nperez0111 if you don't mind. I can add an example and tests as you said. But wouldn't it get picked up by the docs automatically? I'm sure we want to avoid that.

@nperez0111
Copy link

Let's continue discussion in this PR @nperez0111 if you don't mind. I can add an example and tests as you said. But wouldn't it get picked up by the docs automatically? I'm sure we want to avoid that.

If it is a new example, it wouldn't get picked up by the docs. The docs have to import the example, so it is fine to include new ones here

@romansp romansp marked this pull request as draft July 26, 2024 21:49
… properly forward complete chain of injected values. fixes ueberdosis#5394

Vue internally uses prototype chain to preserve injects across the entire component chain. Thus should avoid Object.assign or spread operator as it won't copy the prototype. All correct provides will be already present on `instance.provides`.
@romansp romansp force-pushed the fix/5394-vue-forward-provide-inject-for-components branch from 7e22771 to a71fca4 Compare July 26, 2024 22:14
@romansp romansp marked this pull request as ready for review July 26, 2024 22:14
@romansp
Copy link
Contributor Author

romansp commented Jul 26, 2024

Added test coverage, this is ready to review. Had to slightly adjust demos/setup/vue.ts to hook into app creation routine so it's possible to set app-level provide. Let me know if that's not ideal.

Copy link

@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.

Really appreciate the additional tests & example. Just the one question & we can get this into the next release

Comment on lines +49 to 53
// Vue internally uses prototype chain to forward/shadow injects across the entire component chain
// so don't use object spread operator or 'Object.assign' and just set `provides` as is on editor's appContext
// @ts-expect-error forward instance's 'provides' into appContext
provides: instance.provides,
}

Choose a reason for hiding this comment

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

Just to make sure, does this work for both the options API & composition API. I'm not familiar enough with vue to know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this will work for both Options and Composition API. Tbh I wrote tests in Composition first and then converted to Options for consistency with other examples.

Also no need to worry about composition vs options at all: Options API is literally implemented on top of Composition primitives.

Choose a reason for hiding this comment

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

Thanks for the confirmation!

@nperez0111 nperez0111 merged commit f7f644f into ueberdosis:develop Jul 29, 2024
14 checks passed
@nperez0111 nperez0111 mentioned this pull request Aug 6, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Bug]: Fails to resolve a value via provide/inject in Vue node view components
2 participants