mobxjs / mobx

Simple, scalable state management.
http://mobx.js.org
MIT License
27.55k stars 1.78k forks source link

Any way to "skip" a reaction when changing state in an action? #2313

Closed maltem-za closed 4 years ago

maltem-za commented 4 years ago

I'm asking / maybe suggesting this as a feature. The closest related things I've found are this SO question and #1657, but my scenario is quite different (and related to reaction rather than autorun).

I have a component that represents the create/edit page for an entity. Among others, the entity has lat and lng properties, and there is also a mapbox-gl map on this page. A user can fill the latitude and longitude directly via the provided form fields or by interacting with the map and creating a Point feature. The form fields and the point on the map are kept in sync (more on that below).

The component class has a simple observable state object and one of its properties is an instance of the associated domain object class. Said domain object is effectively "bound" to the HTML form on the page (the form fields get their values from the domain object). The aforementioned synchronisation works as follows: the lat and lng fields, as well as the map, have change handlers that update the domain object as an action, with a corresponding reaction that watches the domain object and updates the map.

This creates a bit of a circular condition when adding the point via the map because user-initiated map change > state update > reaction > programmatic map update. The result is basically just an inconvenience (the Point feature loses its "editable" state because it is immediately re-added programmatically), but I've added conditional logic to the reaction effect function to prevent this.

Here is a simplified version in code (I'm omitting some React boilerplate and other things that aren't important for the purpose of this question):

class ThingyEdit extends React.PureComponent {
    simpleState = {
        thingy: null
    };

    constructor() {
        this.centrePointChangeReactionDisposer = reaction(
            () => {
                return this.centrePoint;
            },
            (centrePoint) => {
                if (!centrePoint) {
                    return;
                }

                if (this.centrePointNeedsToBeUpdated(centrePoint)) { // this is the conditional logic I added
                    this.removeAllPointFeatures();
                    this.drawControl.add(centrePoint);
                }
            },
        );
    }

    componentDidMount() {
        // does the initial assignment of this.simpleState.thingy
        // does map initialisation
    }

    get centrePoint() {
        if (!this.simpleState.thingy || !(this.simpleState.thingy.lng && this.simpleState.thingy.lat)) {
            return null;
        }
        return {
            type: "Point",
            coordinates: [this.simpleState.thingy.lng, this.simpleState.thingy.lat],
        };
    }

    handleFormSubmit = async (event, errors, values) => {
        runInAction(() => {
            this.simpleState.thingy.updateFromJson(values);
        });

        await this.simpleState.thingy.save();
    }

    onLatLngFieldChange = (e) => {
        runInAction(() => {
            this.simpleState.thingy[e.target.name] = e.target.value;
        });
    };

    handleMapDrawEvent(e, isCreate = false, isDelete = false) {
        const featureGeometry = e.features[0].geometry;
        if (featureGeometry.type === "Point") {
            if (isDelete) {
                runInAction(() => {
                    this.simpleState.thingy.lng = "";
                    this.simpleState.thingy.lat = "";
                });
            }
            else {
                if (isCreate) {
                    this.removeAllPointFeatures([e.features[0].id]);
                }

                const [lng, lat] = featureGeometry.coordinates;
                runInAction(() => {
                    this.simpleState.thingy.lng = lng;
                    this.simpleState.thingy.lat = lat;
                });
            }
        }
    }

    render() {
        return (
            <div className="page">
                <AvForm onSubmit={this.handleFormSubmit}>
                    <AvField
                        name="lat"
                        value={this.simpleState.thingy && this.simpleState.thingy.lat}
                        onChange={this.onLatLngFieldChange}
                    />
                    <AvField
                        name="lng"
                        value={this.simpleState.thingy && this.simpleState.thingy.lng}
                        onChange={this.onLatLngFieldChange}
                    />
                </AvForm>
            </div>
        );
    }
}
decorate(ThingyEdit, {
    simpleState: observable
});
export default observer(ThingyEdit);

In summary, it would be convenient if in handleMapDrawEvent I could perform those actions and somehow instruct MobX not to run the associated reaction, as the map is already guaranteed to be up to date at that point.

Thankfully drawing on the map programmatically doesn't raise the associated draw events, which would mean true circularity and more conditional code. It is however easy to imagine such a scenario, and so the question about skipping a reaction becomes more interesting.

I appreciate that this is perhaps not the best way for this component to work, and that certain things can and maybe should be done differently. While I'm open to other ideas or recommendations, that should probably be a separate conversation, and the reality is that this is part of a larger refactoring task to introduce MobX to an existing code base (which is to say that we're trying not to change everything all at once).

mweststrate commented 4 years ago

use a custom comparison function? It is one of the options to reaction

On Mon, Mar 16, 2020 at 12:16 PM Malte Meister notifications@github.com wrote:

I'm asking / maybe suggesting this as a feature. The closest related things I've found are this SO question https://stackoverflow.com/questions/45362610/mobx-autorun-running-too-often-need-it-to-skip-in-some-cases-run-only-after and #1657 https://github.com/mobxjs/mobx/issues/1657, but my scenario is quite different (and related to reaction rather than autorun).

I have a component that represents the create/edit page for an entity. Among others, the entity has lat and lng properties, and there is also a mapbox-gl map on this page. A user can fill the latitude and longitude directly via the provided form fields or by interacting with the map and creating a Point feature. The form fields and the point on the map are kept in sync (more on that below).

The component class has a simple observable state object and one of its properties is an instance of the associated domain object class. Said domain object is effectively "bound" to the HTML form on the page (the form fields get their values from the domain object). The aforementioned synchronisation works as follows: the lat and lng fields, as well as the map, have change handlers that update the domain object as an action, with a corresponding reaction that watches the domain object and updates the map.

This creates a bit of a circular condition when adding the point via the map because user-initiated map change > state update > reaction > programmatic map update. The result is basically just an inconvenience (the Point feature loses its "editable" state because it is immediately re-added programmatically), but I've added conditional logic to the reaction effect function to prevent this.

Here is a simplified version in code (I'm omitting some React boilerplate and other things that aren't important for the purpose of this question):

class ThingyEdit extends React.PureComponent { simpleState = { thingy: null };

constructor() {
    this.centrePointChangeReactionDisposer = reaction(
        () => {
            return this.centrePoint;
        },
        (centrePoint) => {
            if (!centrePoint) {
                return;
            }

            if (this.centrePointNeedsToBeUpdated(centrePoint)) { // this is the conditional logic I added
                this.removeAllPointFeatures();
                this.drawControl.add(centrePoint);
            }
        },
    );
}

componentDidMount() {
    // does the initial assignment of this.simpleState.thingy
    // does map initialisation
}

get centrePoint() {
    if (!this.simpleState.thingy || !(this.simpleState.thingy.lng && this.simpleState.thingy.lat)) {
        return null;
    }
    return {
        type: "Point",
        coordinates: [this.simpleState.thingy.lng, this.simpleState.thingy.lat],
    };
}

handleFormSubmit = async (event, errors, values) => {
    runInAction(() => {
        this.simpleState.thingy.updateFromJson(values);
    });

    await this.simpleState.thingy.save();
}

onLatLngFieldChange = (e) => {
    runInAction(() => {
        this.simpleState.thingy[e.target.name] = e.target.value;
    });
};

handleMapDrawEvent(e, isCreate = false, isDelete = false) {
    const featureGeometry = e.features[0].geometry;
    if (featureGeometry.type === "Point") {
        if (isDelete) {
            runInAction(() => {
                this.simpleState.thingy.lng = "";
                this.simpleState.thingy.lat = "";
            });
        }
        else {
            if (isCreate) {
                this.removeAllPointFeatures([e.features[0].id]);
            }

            const [lng, lat] = featureGeometry.coordinates;
            runInAction(() => {
                this.simpleState.thingy.lng = lng;
                this.simpleState.thingy.lat = lat;
            });
        }
    }
}

render() {
    return (
        <div className="page">
            <AvForm onSubmit={this.handleFormSubmit}>
                <AvField
                    name="lat"
                    value={this.simpleState.thingy && this.simpleState.thingy.lat}
                    onChange={this.onLatLngFieldChange}
                />
                <AvField
                    name="lng"
                    value={this.simpleState.thingy && this.simpleState.thingy.lng}
                    onChange={this.onLatLngFieldChange}
                />
            </AvForm>
        </div>
    );
}

} decorate(ThingyEdit, { simpleState: observable }); export default observer(ThingyEdit);

In summary, it would be convenient if in handleMapDrawEvent I could perform those actions and somehow instruct MobX not to run the associated reaction, as the map is already guaranteed to be up to date at that point.

Thankfully drawing on the map programmatically doesn't raise the associated draw events, which would mean true circularity and more conditional code. It is however easy to imagine such a scenario, and so the question about skipping a reaction becomes more interesting.

I appreciate that this is perhaps not the best way for this component to work, and that certain things can and maybe should be done differently. While I'm open to other ideas or recommendations, that should probably be a separate conversation, and the reality is that this is part of a larger refactoring task to introduce MobX to an existing code base (which is to say that we're trying not to change everything all at once).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx/issues/2313, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBE3SF6MYPTSZJXPHX3RHYKBLANCNFSM4LME4SKQ .

maltem-za commented 4 years ago

@mweststrate 🤦‍♂ Thank you, it seems so obvious now.

I don't know why I didn't consider that before. Maybe because the docs make it sound a bit like I can only work with what comes from the data function: "function will be used to compare the previous and next values produced by the data function". Of course there is nothing preventing me from calling my centrePointNeedsToBeUpdated function too.

lock[bot] commented 4 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.