Skip to content

Using DOM Nodes instead of strings#80

Open
STRd6 wants to merge 1 commit into
NeXTs:masterfrom
STRd6:master
Open

Using DOM Nodes instead of strings#80
STRd6 wants to merge 1 commit into
NeXTs:masterfrom
STRd6:master

Conversation

@STRd6

@STRd6 STRd6 commented Sep 3, 2016

Copy link
Copy Markdown

I'm making a spreadsheet app and rendering input elements inside table rows. The input values and event listeners were getting clobbered by being converted to strings and back.

In this PR I'm caching the real nodes and inserting/removing them. It works great for my purposes and may be of use to others but I understand if there may be reasons why the strings approach was taken.

It seems to work well in my limited test, but there's still an issue with the change on line 174 where we compute the height... I'm assuming the first three rows exist.

Anyway, thought I'd put up these changes in case you found them of interest!

@bharatpatil

Copy link
Copy Markdown

I had also felt the need of storing actual dom nodes. however I'm also curious to understand why string approach has taken. @STRd6 Can you check how much memory your browser tab is consuming?

@NeXTs

NeXTs commented Sep 3, 2016

Copy link
Copy Markdown
Owner

Hello

Thanks! This may be convenient for someone. I remember someone had already requested such feature.
Yep, @bharatpatil, the problem is in memory allocation.

Compare this two tabs in chrome's task manager
strings, nodes
I see almost 5x difference: 72K vs 348K

Because of that I can't replace main library with your implementation @STRd6, but we definitely can move it to separate branch and maintain there. I am guessing there are may be very specific situations when speed is more important than concerns about the memory

p.s. I like how easily you went through sources. changes with minimal interference 👍

regarding height
this.html([rows[0], rows[1], rows[2]]);
single row should be enough
this.html([rows[0], rows[0], rows[0]]);

@STRd6

STRd6 commented Sep 3, 2016

Copy link
Copy Markdown
Author

Using the same node rows[0] will only insert it once rather than three copies, another fix would be to check if (rows.length < 3) return; or however many is needed to accurately guess the sizes.

Comment thread clusterize.js
content_elem.innerHTML = data;

this.empty(content_elem);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey!

How about the following?

var fragment = document.createDocumentFragment();
var dataLength = data.length;

for (var i = 0; i < dataLength; i++) {
    fragment.appendChild(data[i]);
}
 content_elem.appendChild(fragment);

The document fragment should be faster

@dkwin

dkwin commented Feb 14, 2017

Copy link
Copy Markdown

@NeXTs

I think this would be nice to merge with the main branch. Would there be a reason why we couldn't support both Strings and actual Dom nodes?

@NeXTs

NeXTs commented Feb 14, 2017

Copy link
Copy Markdown
Owner

yeah, there is a reason, read previous comments

@timwis

timwis commented Apr 16, 2017

Copy link
Copy Markdown

Hey @STRd6 sounds like we're working on similar apps (here's the one I'm working on). I was hoping to do the same thing, as I'm using choo/bel/yo-yo, which creates dom elements. Curious that it's 5x slower though.

@dumblob

dumblob commented Oct 24, 2021

Copy link
Copy Markdown

@timwis did you manage to make it faster actually?

@timwis

timwis commented Oct 25, 2021

Copy link
Copy Markdown

I'm afraid I have absolutely no memory of this. Sorry!

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.

7 participants