Event pooling + async decorators

[jneuendorf]

General Information

• [ ] Bug
• [ ] Improvement
• [ ] Feature
• [x] Other

Description

I have a question. According to https://stackoverflow.com/a/28046731/6928824 event pooling may be a problem for the throttle/debounce decorators. In your README example you use the debounce decorator for event handling so I guess it has worked for you so far.

So wouldn't it be better to call event.persist() before running the decorated function (I haven't found any code doing that)? Maybe your example works because the event is cleaned after a longer time - so debounce(10e4) would not work...I haven't played around it yet.

Versions

• react-decoration: 2.0.0
• Browser: -

[jneuendorf]

I currently work around this potential problem like so:

import React from "react"
import {debounce} from "react-decoration"

class MyComponent extends React.PureComponent {
    render() {
        const {value} = this.props
        return <input
            type="text"
            value={value}
            onChange={this.onChange}
        />
    }

    onChange = event => {
        // No need to call `event.persist()` because we immediately retrieve the value.
        const {target: {value}} = event
        this.fetch(value)
    }

    @debounce(200)
    fetch(value) {
        const {fetchData} = this.props
        fetchData(value)
    }
}

export {Search}
export default Search

[mbasso]

Hi @jneuendorf, thank you for opening this issue, I really appreciate it.

According to https://stackoverflow.com/a/28046731/6928824 event pooling may be a problem for the throttle/debounce decorators.

I've tested them and it seems that, unfortunately, they have the same problem as underscore or lodash with the instances of the component.

In your README example you use the debounce decorator for event handling so I guess it has worked for you so far.

Yeah, because I always use them to handle user interactions (I used to decorate onChange callbacks for example) I've never figured it out.

So wouldn't it be better to call event.persist() before running the decorated function (I haven't found any code doing that)?

I think that we should add some tests and fix this issue. Let me know if you are interested in contributing 😄

[jneuendorf]

Hey @mbasso,

I just realized that debouncing an event handler is actually a special case because any function can be debounced. So I guess my "workaround from above" would be a valid solution.

Other convenient ways I can think of are

• adding new decorators like debounceEvent or
• checking if the only argument is an instance of SyntheticEvent (I haven't found a way though) or
• adding new event decorator that can be chained with any other decorator and basically calls event.persist() and passes the event to the next decorator.

I guess the last option is the best because it provides most flexibility, is explicit and easy to implement (using your getEventPreprocessor).

Anyway, I would like to contribute stuff ;) And we should definitely write a failing test first (this official example looks like a good start).

[mbasso]

Yeah, I think that the last option would be fine, in this way we can use it both with or without the debounce decorator. We should create a new .md for the new decorator but also point the problem out clearly in the debounce documentation. What do you think about it?

Anyway, I would like to contribute stuff ;)

Awesome! Thank you so much for your interest in this library! 😄

And we should definitely write a failing test first (this official example looks like a good start).

Sure. I think that we should write a test like this and also a test to solve the following problem with the instance (from you link):

var SearchBox = React.createClass({ debouncedMethod: debounce(function () {...},100), }); This is a little bit tricky here. All the mounted instances of the class will share the same debounced function, and most often this is not what you want!. See JsFiddle: 3 instances are producting only 1 log entry globally. You have to create a debounced function for each component instance, and not a singe debounced function at the class level, shared by each component instance.

[jneuendorf]

Hey,

I have done some research (https://www.youtube.com/watch?v=dRo_egw7tBc) on event pooling but could not get the event to be released to the pool. This might be related to react-dom and its test utils: Maybe the test utils just don't behave exactly like the real DOM. I'll try it in the browser and see if it makes a difference.

Since event pooling is done for performance reasons it might not be a good idea to persist the event unless one actually needs a reference to the instance. Thus it would be better to use debounce in combination with an extract... decorator (i.e. extractValue) to just pass the stuff that is really required. This should be included in the new .md file, I guess. For the rare use cases requiring an event instance I would suggest the name persist or persistEvent (the 2nd one is more consistent with killEvent).

I have not looked into the multiple-instances-share-debounced-method problem yet ;)

[jneuendorf]

The real DOM in the browser causes event pooling to behave like in the React docs. Locally both versions of an onChange handler method worked for me:

@persistEvent
@debounce(100)
onChange(event) {
  console.log(event.type);
}

@extractValue
@debounce(100)
onChange(value) {
  console.log(value);
}

[mbasso]

Since event pooling is done for performance reasons it might not be a good idea to persist the event unless one actually needs a reference to the instance. Thus it would be better to use debounce in combination with an extract... decorator (i.e. extractValue) to just pass the stuff that is really required. This should be included in the new .md file, I guess. For the rare use cases requiring an event instance I would suggest the name persist or persistEvent (the 2nd one is more consistent with killEvent).

Cool, it looks good to me, I think that this is the right way to do that. We should certainly clarify this in the documentation. persistEvent is definitely more consistent and clear, it points out that it refers to events.

Locally both versions of an onChange handler method worked for me

Awesome, it's perfect! Exactly what I had in mind

I have not looked into the multiple-instances-share-debounced-method problem yet ;)

I'll try to create a snippet of this issue in the next few days. I think that this it might be not to easy to fix, it might create some problems... We should try the 3 solutions proposed on stackoverflow, from the first to the last one. If you want, feel free to open a Work In Progress PR 😄

[jneuendorf]

I'll try to create a snippet of this issue in the next few days. I think that this it might be not to easy to fix, it might create some problems... We should try the 3 solutions proposed on stackoverflow, from the first to the last one.

Wouldn't it be sufficient to internally use the @autobind decorator? So basically instance would get method.bind(this)?

If you want, feel free to open a Work In Progress PR

What do you mean by Work In Progress PR? I though I open a PR once stuff is getting close to being final. Oh, I just remembered GitHub now supports squashed PRs, right?

Update

It might really be a bit difficult to have 1 debounce function per instance because autobind achieves getting the correct reference to this (at runtime) by using a getter on the descriptor whereas (almost?) all of this library's decorators use descriptor.value (at class-load time). Thus we would also have to use a getter that wraps e.g. the debouncer function. This itself is not a problem but since all other decorators assume descriptor.value is the function to wrap, this would break decorator chaining. 😢

The trivial solution would be changing all decorators to do their wrapping at runtime but this is less performant, of course. 😕

Update 2

The non-trivial solution would be detecting whether the descriptor has a getter or a value. This would be quite a refactoring, I guess.

[mbasso]

I'm sorry for my late reply but I have been a little bit busy in last days...

What do you mean by Work In Progress PR? I though I open a PR once stuff is getting close to being final. Oh, I just remembered GitHub now supports squashed PRs, right?

yeah, just a PR to see the progress and review the code together. It's also fine to open it once stuff is getting close to being final, as you said. As you prefer

The non-trivial solution would be detecting whether the descriptor has a getter or a value. This would be quite a refactoring, I guess.

This seems interesting, I think that it is certainly something that needs to be done... It might involve some changes in the codebase and new tests but it will represent a big improvement for the library.

The only solution that I had in mind was the third:

var SearchBox = React.createClass({
    method: function() {...},
    componentWillMount: function() {
       this.method = debounce(this.method,100);
    },
});

but componentWillMount is now considered legacy, so we cannot overwrite it to make the decorator work... Maybe we can overwrite the constructor:

class SearchBox extends React.Component {
    constructor(props) {
        super(props);
        this.method = debounce(this.method,1000);
    }
    method() { ... }
}

but I think that it will break decorator chaining as well.

I have no other ideas in mind at the moment, it seems that your non-trivial solution is the best.

[mbasso]

Hi @jneuendorf, news about this?

[jneuendorf]

@mbasso Sorry I haven't been working on this lately...I was busy with other projects but I haven't forgotten about this! 😉 I'll open the WIP PR within the next couple days

[jneuendorf]

@mbasso I've done it! 😄 Though, I haven't written any tests for the new debounce yet - just jotted it down before going to bed 😉

[mbasso]

@jneuendorf awesome! Thank you so much for your effort 😄 I'll read the code as soon as possible!