nareshbhatia / mobx-state-router

MobX-powered router for React apps
https://nareshbhatia.github.io/mobx-state-router/
MIT License
227 stars 30 forks source link

Preserve the context of route's hooks #74

Closed kirilldronkin closed 4 years ago

kirilldronkin commented 4 years ago

Hi, thank you for your work. I suggest don't destructuring of onEnter, onExit etc. hooks during a transition to preserve their context for a more convenient usage of routes as classes. Currently I have to apply @autobind:

import {Route, RouterState} from 'mobx-state-router';
import autobind from 'autobind-decorator';

class ReportRoute implements Route {
    public name = ReportRoute.NAME;
    public pattern = `/report`;

    @autobind public onEnter(from: RouterState, to: RouterState) {
        // ...
    }

    static NAME = 'report';
}
nareshbhatia commented 4 years ago

Hi @kirilldronkin, I am glad that you liked my work.

I don't understand what you mean by destructuring of hooks. Hooks are just pure functions supplied to the library and called during transitions: see here. Also they are meant to be simple JavaScript objects, not classes.

What are you trying to achieve here? May be there is a different way to do what you want to do.

kirilldronkin commented 4 years ago

You can use classes as well as objects to represent a route, thank to TypeScript's structural typing. In my project routes are instantiated and passed to the store via DI and incapsulate some logic about stores initializing and further observing. Here is a simplified example of one of such routes:

class ReportRoute extends Disposable implements Route {
    @inject reportStore: ReportStore;

    name = ReportRoute.NAME;
    pattern = `/report/:type(${Object.values(ReportType).join('|')})`;

    async onEnter(from: RouterState, to: RouterState): Promise<void> {
        const type = TypeParam.decode(to.params.type) ?? null;

        this.reportStore.setType(type);

        this.disposers.push(
            reaction(
                () => this.reportStore.type,
                (type) => {
                    // React somehow on the type change
                }
            )
        );
    }

    async onExit(): Promise<void> {
        this.dispose();
    }

    static NAME = 'report';
}

If you replace destructuring (e.g. const { beforeExit, onExit } = fromRoute) to direct call (e.g.: fromRoute.beforeExit()) my config will work like a charm without usage of @autobind.

nareshbhatia commented 4 years ago

That should be easy enough! Can you confirm that you are talking about only these two lines? I could simply rewrite the logic as:

if (fromRoute.beforeExit) {
    redirectState = await fromRoute.beforeExit(fromState, toState, this);
    ...
}

Please note that I released version 5.0.0 this morning. If I do the change it would be in this version. Please see the change log and revised docs for more information.

kirilldronkin commented 4 years ago

Yes, you are right, by not doing destructuring this in the hooks is preserved and we can use classes safely.

Ok, got it. BTW v5 is wonderful, I'm very glad that the library is evolving.

nareshbhatia commented 4 years ago

Fixed. Please see v5.0.1