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

Network requests on each render #26

Open
arturi opened this issue May 26, 2016 · 18 comments
Open

Network requests on each render #26

arturi opened this issue May 26, 2016 · 18 comments

Comments

@arturi
Copy link

arturi commented May 26, 2016

Hi! Thanks for the module.

I have an example here: http://requirebin.com/?gist=f2a99cc27c465ed51eb8159830836ca4. In it, each time I call update and bel creates a new element, a local network request is made for the base64 inlined image, even though it did not change.

Should this be happening? I guess it makes sense, new element is created, bel is fetching it’s content. But that results in a poor performance — when I add a relatively large image, like 3mb, the Chrome browser tab can barely function and the interface becomes unusable.

To reproduce: navigate to http://uppy.io/examples/modal/index.html, click “open modal” and drag two files to the dragdrop area. Open network tab, click “upload”.

What am I doing wrong? Thank you!

@yoshuawuyts
Copy link
Member

In itself it shouldn't be the worst the worst because the image is read out from cache each time, but I can imagine that you'd want to evade this scenario as much as possible regardless. You're indicating chrome freezes up for large images and that's definitely bad :(

An approach you can take is to memoize / wrap your function in a thunk. Fancy words for something that looks a bit like this:

var imgEl = null
function render (time) {
  if (!imgEl) imgEl = html`<img src="${img}">`
  return html`<div>
    <h1>${time}</h1>
    ${imgEl}
  </div>`
}

That way you no longer create a new request each time a render is passed. This should also work to optimize large sections of a tree, based on say changes in application state or other properties being passed.

I hope this was helpful. Cheers!

@arturi
Copy link
Author

arturi commented May 26, 2016

@yoshuawuyts thank you! your solution works ✨ (proof: http://requirebin.com/?gist=0f7bb3b03959448e120f02d506f4cd7e)

However, made me think and try other solutions:

  1. React: http://esnextb.in/?gist=ec75c4fe3443dc16920a6d740415c090
  2. Virtual-DOM + hyperx + main-loop: http://requirebin.com/?gist=ecda5235da884da9335eb8029ccdcff9

Both work fine, no additional network requests. Which makes me wonder why the same won’t just work with bel. virtual-dom is smarter about creating new elements?

@shama
Copy link
Member

shama commented May 26, 2016

This is an interesting problem! Both React and virtual-dom have access to the previous value. So they can check if the values are the same and skip the operation. bel doesn't have access to the previous value so it can't check if it's the same as last time to ignore the operation. It only creates elements and relies on morphdom or any other native DOM diffing library to handle the update.

For example we can't do the following when creating an element:

var previous = element.getAttribute(prop)
if (value !== previous) element.setAttribute(prop, value)

Because the element is always new and setting a src on a img element will initiate a network request.


AFAIK, appending to a document.createDocumentFragment() will still initiate the network request too.

I wonder if we should handle certain properties that can initiate requests or side effect events specially somehow. Or if we'll have to use an element registry. Or just leave it up to the framework implementations to handle this?

@jamesplease
Copy link

The solution proposed by @yoshuawuyts is great, and seems like it would work well, but I think it'd be nice to make the problem hidden from developers. I'm not sure if that belongs in bel or a framework tho'.

Aside from src, are there other attributes with side effects that happen immediately upon being set?

@yoshuawuyts
Copy link
Member

but I think it'd be nice to make the problem hidden from developers

Don't think that's possible - controlling re-renders is something that comes with the job I'm afraid. Be it through shouldComponentUpdate in React, manual diffing using thunks or setting lifecycle hooks to load and unload data. I reckon the best we can do is document it and make it as transparent as possible - that way we keep control which is a great thing.

I'll add docs for this in choo, as I reckon you're right and this is definitely a framework-level concern too. Thanks!

@arturi
Copy link
Author

arturi commented May 27, 2016

Don't think that's possible - controlling re-renders is something that comes with the job I'm afraid

But, from a user/developer perspective, virtual-dom allows me to not think about this problem at all, meaning one less reason to use yo-yo and bel, even though I like them and appreciate the “real DOM instead of a virtual-dom” thing.

/cc @maxogden @substack, what do you think?

#tinylibrariesfatigue

@shama
Copy link
Member

shama commented May 27, 2016

Each contributor has different motivations but for me, I couldn't care less about competing with React or virtual-dom. I use yo-yo because it solves a very specific problem for me: I hate peer dependencies.

At work we use Ember, it's a massive app with over 400 components. It has literally taken a year and wading through a ton of merge conflicts to finally upgrade to 1.13. The latest Ember is now 2.5 so who knows if our app will ever actually catch up.

This isn't Ember's fault, it's an inherit problem with peer dependencies when you maintain a large number of components. React and virtual-dom have this issue too. One could argue the free upgrades we get from the framework makes it worth it. It's hard to actually know if all that time spent keeping up would have been better specifically improving our components instead.

Outside of work, I don't have time to keep up with a framework. So I use yo-yo for everything. I can leave a project for a year and assuming the browser didn't break my code, it will still work with new modules I create.

I know this doesn't solve this particular issue but thought I'd share my own motivation behind the native DOM approach.

@arturi
Copy link
Author

arturi commented May 31, 2016

I’m totally on board with your motivation. We went with yo-yo in Uppy because we are building a file uploading library and wanted something simple and lightweight. And we’ve been generally happy with it, so thanks again. I plan to use it in my personal projects as well, once I get a hold of things.

@max-mapper
Copy link
Collaborator

I think it would be great to hide this from developers. Obviously what @yoshuawuyts said about this coming with the territory is part of working with the real DOM, but also I think thats a cop-out answer when we haven't explored all the possibilities.

Two possible workarounds:

  • wrap everything in a <noscript> tag before passing to morphdom, which disables all scripts as well as image loading, then have morphdom only operate on the inner tree (bypassing the tag)
  • rewrite the img url to be something like morphdom-http://originalurl.com and then have morphdom rewrite it when it gets inserted into the dom

@yoshuawuyts
Copy link
Member

yoshuawuyts commented Jun 25, 2016

@maxogden I'd be careful with tying bel to morphdom specifically; the more native DOM abstractions we use, the more generic and thus reusable the solution is. While i see a place for the solutions you're proposing, they wouldn't, for example, work with React's render loop or any other vdom implementation.

Right now morphdom seems to bench on par with react - with virtual-dom sitting at 2x the speed of that; and the fastest vdom implementation benching way above that. While morphdom works great right now; I think we can all agree that it'd be great if we can finally get around to writing browser elements that outlast more than a single cycle of frameworks.

edit: that's not to say that I don't agree with you that it would be great if we could abstract this away from devs; we should def explore these options! What I'm trying to say is we should be cautious in using abstractions that diverge from the DOM, as relying on them might come back to bite us

@max-mapper
Copy link
Collaborator

@yoshuawuyts why wouldn't wrapping it in a <noscript> work everywhere? is a 'native DOM' element. should work in any real/virtual DOM thing right?

@max-mapper
Copy link
Collaborator

BTW made an experiment to test the noscript theory and I can't get it to disable requests in chrome or FF so maybe that isn't even worth discussing further http://requirebin.com/?gist=8dc9eebaefdb1ca0d04684c7095619d6

@yoshuawuyts
Copy link
Member

why wouldn't wrapping it in a work everywhere? is a 'native DOM' element. should work in any real/virtual DOM thing right?

Oops; I interpreted it as though morphdom would inject behavior surrounding this - which wouldn't be transparent. If morphdom isn't the one adding this beahvior I'm def all 👍

@arturi
Copy link
Author

arturi commented Aug 6, 2016

Found another problem with the caching img element solution to prevent extra requests. For some reason when using pre-created element saved in a variable, entire element tree where that pre-created element is then used gets re-rendered in DOM.

Please see here: http://requirebin.com/?gist=0f7bb3b03959448e120f02d506f4cd7e. If you use inspector to compare behaviour of the two img elements — the first one gets thrown away by morphdom, preventing animations (the first animation can’t finish) and click events (which might fire in between those DOM changes). The second stays in place like it should.

@yoshuawuyts
Copy link
Member

Think this might be related to shama/on-load#10, which seems like an issue in morphdom if I understood the thread well

@timwis
Copy link
Member

timwis commented Aug 6, 2016

Reported to morphdom at patrick-steele-idem/morphdom#77

@arturi
Copy link
Author

arturi commented Aug 12, 2016

What do you think of this solution by to the “src network requests” problem: patrick-steele-idem/morphdom#77 (comment)?

@yoshuawuyts
Copy link
Member

Waiting on patrick-steele-idem/morphdom#77 to be published; made a demo on how to create widgets with the isSameNode() API here: shama/on-load#10 (comment) (e.g. thunking!)

That patch should fix this issue 🎉

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

No branches or pull requests

6 participants