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

convertHtmlToDelta does not create the exact delta that quill works with #3

Open
Xiphe opened this issue Nov 15, 2018 · 11 comments · May be fixed by #4
Open

convertHtmlToDelta does not create the exact delta that quill works with #3

Xiphe opened this issue Nov 15, 2018 · 11 comments · May be fixed by #4
Assignees

Comments

@Xiphe
Copy link

Xiphe commented Nov 15, 2018

Given I paste <h2>Hello</h2><p>World</p> into a quill instance running in a real browser. getContents will return

{
  ops: [
    { insert: "Hello" },
    { attributes: { header: 2 }, insert: "\n" },
    { insert: "World\n" }
  ]
}

while convertHtmlToDelta returns

{
  ops: [
    { attributes: { header: 2 }, insert: "Hello\n" },
    { insert: "World\n" }
  ]
}

This cause problems with dirty-checks when the contents of quill are used in a <form>.

Will submit a PR.

@Xiphe Xiphe linked a pull request Nov 15, 2018 that will close this issue
Xiphe added a commit to Xiphe/node-quill-converter that referenced this issue Nov 15, 2018
by setting delta as content to quill and then returning the content

fix joelcolucci#3

BREAKING CHANGE:
This likely alters the result of any convertHtmlToDelta conversion.
Depending of what people are doing with the delta this might break things.
@joelcolucci
Copy link
Owner

Hi @Xiphe, Thanks for submitting this issue (and PR). I'll take a look at this shortly and will follow up! Thank you for your patience and work. - Joel

@joelcolucci joelcolucci self-assigned this Nov 20, 2018
@joelcolucci
Copy link
Owner

@Xiphe I apologize for the delay. I'll be reviewing this, this week!

@joelcolucci
Copy link
Owner

I can confirm the input/output provided for convertHtmlToDelta .

Input:

'<h2>Hello</h2><p>World</p>'

Resulting Output:

{
  "ops": [
    { "attributes": { "header": 2 }, "insert": "Hello\n" }, 
    { "insert": "World" }
  ]
}

@Xiphe Can you provide the following for when you experience the issue in browser?

  • Version of Quill
  • Browser
  • Browser version
  • Operating system

@Xiphe
Copy link
Author

Xiphe commented Dec 23, 2018

Latest Quill
Latest Chrome
Latest OSX

@joelcolucci
Copy link
Owner

Hi @Xiphe , Can you send me the semvers for each?

@Xiphe
Copy link
Author

Xiphe commented Dec 30, 2018

Hey @joelcolucci, in the meantime we stopped using this package since there seems to be a memory leak (which we did not debug properly, so I can not tell you where and why).

Versions related to this issue:

"quill": "^1.3.6"
macOS: 10.14.1
Chrome: Version 71.0.3578.98 (Official Build) (64-bit)

From my side you can also just close this + #4 - It's up to you.

@joelcolucci
Copy link
Owner

Thanks @Xiphe . I appreciate the feedback and update. This month I am committing more time to this project.

Are you able to share at what scale the memory leak occurred at? As in converting 1000, 10000, 100000 quills etc?

Thank you again for your time.

@Xiphe
Copy link
Author

Xiphe commented Jan 3, 2019

Are you able to share at what scale the memory leak occurred at.

As in not converting at all but loading the module. But as I said, we have not digged any deeper. Could also be a not relate to this module and we interpreted s.th. wrong.

Maybe @fgnass knows a little more here.

@jessewilliams1
Copy link

+1 on the memory leak. Just figured that out after simply loading the module.

node.js 10.10.0
npm 6.4.1
express: 4.17.1

@paramjeetkumar7297470
Copy link

paramjeetkumar7297470 commented Nov 19, 2019

@joelcolucci Is memory leak issue resolved now? I have to convert HTML to Delta on node server side. So i need this package to implement on node side

@joelcolucci
Copy link
Owner

Hey all. Just published version 0.3.3 which should resolve the memory leak from importing the module. See #10 for updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants