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: configure should use the parent of the current instance, to avoid duplication #5147

Merged
merged 1 commit into from
May 14, 2024

Conversation

nperez0111
Copy link
Contributor

@nperez0111 nperez0111 commented May 14, 2024

Changes Overview

Based on the approach taken by #5136 and validated for appropriate behavior with a barrage of tests.

The gist of what is happening here is that extend() will always set the new child's parent property to be this (establishing a child -> parent relationship).

When doing .configure() it is doing a .extend(). Doing this directly on an extension (i.e. one that is not already .extend'd), as it will create a child -> parent relationship with pretty much the exact same config so it ends up overwriting itself with it's same options.
But what we really want is not a child -> parent relationship, but to just overwrite some options while maintaining an effective "copy" of the child (i.e. preserving it's child -> parent relationship).

This is pretty complicated, so I'm going to add a diagram explaining the current behavior and desired behavior here:

what it was

should be this

There was a lot of digging for this one:

It should resolve:

It may resolve:

Implementation Approach

Someone had implemented this already: #5136
I just validated that it worked and made sense with a barrage of tests. The root behavior I was trying to fix was #4704

Testing Done

Most of the code was adding tests to make sure that I understood what the problem was and if it fixed it.

Verification Steps

Run the tests with the Extension.ts file as it was before and note the tests that it fails to pass now without it

Additional Notes

Checklist

  • I have renamed my PR according to the naming conventions. (e.g. feat: Implement new feature or chore(deps): Update dependencies)
  • 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

Copy link

netlify bot commented May 14, 2024

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 8cbd8e2
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/66432eaf88c0dd00082ecdb9
😎 Deploy Preview https://deploy-preview-5147--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.

Copy link
Contributor

@bdbch bdbch left a comment

Choose a reason for hiding this comment

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

Nice and easy fix. looks good to me!

@634750802
Copy link

I think it's same for Nodes and Marks?

@nperez0111
Copy link
Contributor Author

I think it's same for Nodes and Marks?

Ah jeez, I missed that. I spent so much time trying to understand it I didn't realize it. Will follow up in another PR @634750802

nperez0111 added a commit that referenced this pull request May 15, 2024
nperez0111 added a commit that referenced this pull request Jun 18, 2024
nperez0111 added a commit that referenced this pull request Jun 18, 2024
nperez0111 added a commit that referenced this pull request Jun 18, 2024
nperez0111 added a commit that referenced this pull request Jun 18, 2024
nperez0111 added a commit that referenced this pull request Jun 18, 2024
* fix: apply #5147 fix to marks and nodes

* fix: resolve Issue #4704 by reverting PR #4191

* test: more robust tests for nodes and marks too
@github-actions github-actions bot mentioned this pull request Jun 28, 2024
@nperez0111 nperez0111 mentioned this pull request Jul 13, 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
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants