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 weird performance issue #139

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sbstjn
Copy link

@sbstjn sbstjn commented May 30, 2018

Hi,
Thanks for the great react-rte! We noticed a few performance issues with the underlying draft-js-import-html and draft-js-import-element libraries. I tried to fix it for "our use case" and added tests that make sure large/weird HTML structures can be parsed.

Existing tests still work fine 🎉

//cc @HenrikFricke

@sbstjn
Copy link
Author

sbstjn commented Jun 1, 2018

This breaks other untested code/cases ;(

@sibelius
Copy link
Collaborator

@sbstjn is this ready to merge?

@@ -500,8 +500,12 @@ function concatFragments(fragments: Array<TextFragment>): TextFragment {
let characterMeta: CharacterMetaSeq = Seq();
fragments.forEach((textFragment: TextFragment) => {
text = text + textFragment.text;
characterMeta = characterMeta.concat(textFragment.characterMeta);

if (!characterMeta.isSuperset(textFragment.characterMeta)) {

Choose a reason for hiding this comment

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

Do you mind explaining how this fixes the large parsing of HTML data?

@sergiu-paraschiv
Copy link

This fix breaks parsing.

For this input:

<p style="">
    <span>Line one.</span>
</p>
<p style="">
    <span>Line two </span><a href="/foo/bar"><span>link</span></a><span>.</span>
</p>
<p style="">
    <span>Line three </span><a href="https://foo.bar"><span>link one</span></a><span> and </span><a href="https://foo2.bar"><span>link two</span></a><span>.</span>
</p>

Notice the text " and " between the links in the third paragraph.

characterMeta is incorrectly built for them and the output marks " and " as part of the second "link" entity which in turn results in DraftJS rendering it as <a href...>link</a> after which it completely omits the remaining five characters.

The bug this is trying to fix is in the processText method: var seq = Repeat(charMetadata, text.length); should actually be var seq = Repeat(charMetadata, 1);. I can't figure out why a Repeat was used here in the first place, but repeating the same sequence of characters the same times as it's length is for sure not correct. Replacing that with var seq = Repeat(charMetadata, 1); fixes the original bug and the fix here (if (!characterMeta.isSuperset(textFragment.characterMeta)) {) is not required.

@sergiu-paraschiv
Copy link

Later: my fix does not work.

@sergiu-paraschiv
Copy link

But this patch does:

diff --git a/node_modules/draft-js-import-element/esm/stateFromElement.js b/node_modules/draft-js-import-element/esm/stateFromElement.js
index eccabfd..d14b574 100644
--- a/node_modules/draft-js-import-element/esm/stateFromElement.js
+++ b/node_modules/draft-js-import-element/esm/stateFromElement.js
@@ -513,8 +513,9 @@ function collapseWhiteSpace(text, characterMeta) {
 
   text = _trimTrailingSpace.text;
   characterMeta = _trimTrailingSpace.characterMeta;
-  var i = text.length;
 
+  characterMeta = characterMeta.toArray();
+  var i = text.length;
   while (i--) {
     if (text.charAt(i) === ' ' && text.charAt(i - 1) === ' ') {
       text = text.slice(0, i) + text.slice(i + 1);
@@ -522,6 +523,8 @@ function collapseWhiteSpace(text, characterMeta) {
     }
   } // There could still be one space on either side of a softbreak.
 
+  characterMeta = Seq(characterMeta);
+
 
   var _replaceTextWithMeta = replaceTextWithMeta({
     text: text,
@@ -561,14 +564,16 @@ function canHaveDepth(blockType) {
 
 function concatFragments(fragments) {
   var text = '';
-  var characterMeta = Seq();
-  fragments.forEach(function (textFragment) {
-    text = text + textFragment.text;
-    characterMeta = characterMeta.concat(textFragment.characterMeta);
-  });
+
+  var characterMeta = [];
+  for (var i = 0; i < fragments.length; i++) {
+      text = text + fragments[i].text;
+      characterMeta.push(...fragments[i].characterMeta.toArray());
+  }
+
   return {
     text: text,
-    characterMeta: characterMeta
+    characterMeta: Seq(characterMeta)
   };
 }
 

Nic0S added a commit to WonderInventions/draft-js-utils that referenced this pull request Feb 6, 2023
Roughtly following the suggestion here sstur#139
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 this pull request may close these issues.

4 participants