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

Using DOM Nodes instead of strings #80

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

Conversation

STRd6
Copy link

@STRd6 STRd6 commented Sep 3, 2016

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

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
Copy link
Owner

NeXTs commented Sep 3, 2016

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
Copy link
Author

STRd6 commented Sep 3, 2016

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.

content_elem.innerHTML = data;

this.empty(content_elem);

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
Copy link

dkwin commented Feb 14, 2017

@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
Copy link
Owner

NeXTs commented Feb 14, 2017

yeah, there is a reason, read previous comments

@timwis
Copy link

timwis commented Apr 16, 2017

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
Copy link

dumblob commented Oct 24, 2021

@timwis did you manage to make it faster actually?

@timwis
Copy link

timwis commented Oct 25, 2021

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.

None yet

7 participants