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 eventListener binding to avoid creating new functions #11

Open
tamb opened this issue Aug 14, 2019 · 6 comments
Open

Fix eventListener binding to avoid creating new functions #11

tamb opened this issue Aug 14, 2019 · 6 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@tamb
Copy link
Owner

tamb commented Aug 14, 2019

Currently bind is being used to preserve context when using addEventListener. The bound function created is then stored in an object $b per component instance. Would like to improve this.

@tamb tamb added the enhancement New feature or request label Aug 14, 2019
@tamb
Copy link
Owner Author

tamb commented Aug 29, 2019

I've decided against this. Will reconsider it when auditing the framework later

@tamb tamb closed this as completed Aug 29, 2019
@tamb tamb reopened this Sep 6, 2019
@tamb
Copy link
Owner Author

tamb commented Sep 6, 2019

This has been brought to my attention again. A fix is in the works

@tamb
Copy link
Owner Author

tamb commented Sep 6, 2019

brought to my attention by a reddit comment

https://www.reddit.com/r/javascript/comments/d0abnn/server_rendered_components_in_under_2kb/ez9pzo1?utm_source=share&utm_medium=web2x

There's some stuff you can consider to slightly optimize a bit more. For each of your components, considering using a ES6 module or classes with static functions. No only can these be tree-shaken easier with webpack or roll up (won't include functions you never reference), but you reduce the RAM usage per component.

Consider the fact you're creating a JavaScript object per component. If you have 100 checkboxes on your page, each checkbox would have an object created for it new CheckboxComponent(element). And each DOM element would bind the change event to a function inside each object element.addEventListener('change', (e) => this.onChange(e)) . So, for 100 DOM elements, 100 browser-handled DOM Event Handlers, 100 JS Objects, and each has 100, technically different, event functions. The RAM starts to add up.

Instead, since consider all 100 DOM checkboxes change event binds to a static function CheckboxComponent. So, instead it's element.addEventListener('change', CheckboxComponent.onChange). As for accessing what element called onChange(e), you can read e.currentTarget. If you need to access per element data, you can use element.dataset (slow), or use a WeakMap<Element, Object>. Doing it this way means when the element is removed from the DOM, the DOM Event Handler disappears, and it's also gone from the WeakMap. There are no lingering JS Objects that you have to cleanup.

I do this for all my applications now and it's pretty close to living and breathing with the DOM. I have an example of this with Material Design framework I built, but I haven't flushed anything too complicated because I haven't tried to replicate a template engine.

I can show you an example of something simple like a button or text field. Or you can look at something more complex like a List or Tab, which has sub-components that interact with each other by passing CustomEvent through the DOM.

@tamb tamb added the help wanted Extra attention is needed label Oct 23, 2019
@tamb
Copy link
Owner Author

tamb commented Dec 5, 2019

I'm going to table this for now

@tamb tamb closed this as completed Dec 5, 2019
@tamb
Copy link
Owner Author

tamb commented Dec 23, 2019

I'm going to reopen this issue and investigate using a weakmap for the binding the methods. I will not be using the static keyword, rather I will be using a weakmap so the disconnecting method works without needing to unbind listeners. This should save start up time.

@tamb tamb reopened this Dec 23, 2019
@tamb tamb self-assigned this Dec 23, 2019
@tamb
Copy link
Owner Author

tamb commented Aug 7, 2020

I believe the option is to use experimental syntax for component classes. I will wait for this to become a standard:
The following will remove the need to use bind

doSomething = () => {}

Using a closure could fix this issue. However, benchmarks have them as equal:
https://jsperf.com/bind-vs-closure-setup/32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant