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

Small questions #2

Open
bcomnes opened this issue Jul 22, 2019 · 2 comments
Open

Small questions #2

bcomnes opened this issue Jul 22, 2019 · 2 comments

Comments

@bcomnes
Copy link
Contributor

bcomnes commented Jul 22, 2019

This should be this.update() instead of this.render() to take advantage of the raf management yeah?

hui/index.js

Lines 82 to 88 in 568c274

_onload () {
this.loaded = true
this.onload()
// if any listeners were attached in onload we trigger a rerender as state
// may have changed between createElement or unload/load
if (this[unloaders].length) this.render()
}

We should shadow bind this._reallyUpdate in the constructor since update could be called at a high frequency (e.g. some kind of animation loop that uses this.update() to render new frames.)

hui/index.js

Lines 53 to 57 in 568c274

update () {
if (this[updating]) return
this[updating] = true
window.requestAnimationFrame(this._reallyUpdate.bind(this))
}

Thoughts?

@emilbayes
Copy link
Member

On the first one: _onload could already be inside a RAF call, that's why it goes directly to this.render() to avoid a flicker of uninitialized state. onload is called when the element enters the DOM anyway, and this is at the cost that it could do a double render in the same frame, but barring the flicker that would otherwise occur.

The second one I don't understand :)

@bcomnes
Copy link
Contributor Author

bcomnes commented Jul 22, 2019

When this.update() is called, it creates bound functions every call, which, (at least historically) has caused memory/perf issues in some cases. Switching it to a predefined bound function on the instance only creates a bound function once, on instantiation.

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

2 participants