joelcolucci / node-quill-converter

Convert HTML to a Quill Delta or a Quill Delta to HTML
MIT License
95 stars 25 forks source link

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

Open Xiphe opened 5 years ago

Xiphe commented 5 years ago

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.

joelcolucci commented 5 years ago

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 commented 5 years ago

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

joelcolucci commented 5 years ago

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?

Xiphe commented 5 years ago

Latest Quill Latest Chrome Latest OSX

joelcolucci commented 5 years ago

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

Xiphe commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

+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 commented 4 years ago

@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 commented 4 years ago

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