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: streamline html package generation functions to work as expected #5257

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

bdbch
Copy link
Contributor

@bdbch bdbch commented Jun 20, 2024

Changes Overview

This PR removes logic that was specific to the @tiptap/html package and didn't work the same way the core generateHTML and generateJSON functions work. This caused issues with a few extensions (for example the Youtube extension not being able to be generated back to HTML from JSON, see #4089)

I kept the original HTML getJSON and getHTML functions to not break existing code for users but maybe we should add a deprecation message to it?

Implementation Approach

I removed the logic of the functions of the html package and just used a reference to the core packages util functions.

Testing Done

I tried running JSON and HTML generation steps before and after and checked if unexpected things were happening or the output was false.

Verification Steps

  1. Try to check out this PR
  2. Create a demo and simply run the code example below
  3. Verify output
import { generateJSON, generateHTML } from '@tiptap/html'
const extensions = [Document, Paragraph, Text, Youtube]
const content `<p>Hello</p><div data-youtube-video><iframe src="..."></iframe></div>`

const json = generateJSON(content, extensions)
const html = generateHTML(json, extensions)

console.log(json, html)

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

@bdbch bdbch requested a review from svenadlung as a code owner June 20, 2024 12:37
@bdbch bdbch self-assigned this Jun 20, 2024
Copy link

netlify bot commented Jun 20, 2024

Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit 1fa718a
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/6674229e72561700080ab53e
😎 Deploy Preview https://deploy-preview-5257--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

Wouldn't this break being able to use @tiptap/html package on the server-side where this is no reference to a global document object 🤔

@bdbch
Copy link
Contributor Author

bdbch commented Jun 21, 2024

True, didn't think about this. I'll add back the zeed fallback if the document is not available but I guess this will also cause the issues from #4089 to pop back up.

@nperez0111
Copy link
Contributor

If you are on the server side anyway I think that we could use another package than zeed-dom. Perhaps happy-dom? It is a heavier package, but we should not be using this package in the browser. We could even give a warning to a user if in development mode and there is a global document object that they should not be bundling this into their application.

@bdbch bdbch changed the base branch from main to develop June 26, 2024 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triage open
Development

Successfully merging this pull request may close these issues.

[Bug]: Cannot parse generated html from YouTube extension
2 participants