Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

actionsDict is not bound to this #2

Closed
esamattis opened this issue May 24, 2018 · 8 comments
Closed

actionsDict is not bound to this #2

esamattis opened this issue May 24, 2018 · 8 comments

Comments

@esamattis
Copy link

esamattis commented May 24, 2018

Ie. cannot call another action via this.

Example:

	const initialState = { s: "" }
    const red = createRedutser(initialState, {
      a(state, act: {}) {
        return this.b(state, act)
      },

      b(state, act: {}) {
        return {
          s: state.s + "b",
        }
      },
    })

This gives weird type errors

image

and also errors in runtime because this is undefined.

I managed to workaround this with following helper:

import {mapValues} from "lodash";

type IState = typeof initialState

type ActionCreators = {
    [key: string]: (state: IState, action: any) => IState;
};

export function bindThis<T extends ActionCreators>(obj: T): T {
    const mapped = mapValues(obj, value => {
        if (typeof value === "function") {
            return value.bind(obj);
        }

        return value;
    });

    return (mapped as any) as T;
}

// Usage

const redutser = createRedutser(
    initialState,
    bindThis({ /* actions */ })
);
@wkrueger
Copy link
Owner

Having a look at it. (Somehow this project got unwatched on my github so i never saw your issues).

@wkrueger
Copy link
Owner

wkrueger commented May 29, 2018

While the binding issue has been fixed, I couldnt find a decent fix for the inference issues without changing createRedutser signature. For now, you can use createRedutser2 when using this in the Dict. Ugly and sad... :(

@esamattis
Copy link
Author

I Might have found a solution for the inference issue. Here's a patch for it

esamattis@a34d87e

This builds on the PR #4.

The idea is to explicitly add typing for this

image

I did not sent PR for this because this breaks the subdomain specs and currently I don't have time to look at it.

@wkrueger
Copy link
Owner

wkrueger commented Jun 1, 2018

I probably have tried something similar and ended up in a loophole where one of the inferred types got to {} (which actually means: I couldnt infer but hey I wont throw you an error).

I'll have a look on your gist, thanks!

In the worst case, ts3.0 is promising partial inference which may fix things that require this curry workaround.

@esamattis
Copy link
Author

I probably have tried something similar and ended up in a loophole where one of the inferred types got to {} (which actually means: I couldnt infer but hey I wont throw you an error).

Do you see this with TS 2.9? Seems to be working for me just fine.

@wkrueger
Copy link
Owner

wkrueger commented Jun 15, 2018

Seems it only happens when you return something from this.
"bindToSelf" test (modified) continues to fail on ts 2.9.

About your suggestion, that erases the this parameter to a generic object instead of receiving the actual object through generics.

I have also tried adding a generic Self parameter to this too (or even adding a 3rd parameter), but that ends up propagating to the whole codebase and still bringing other new issues (ex: circular references).

So, ugly as it seems, createRedutser2 still is currently more elegant than the other solutions. I could bring a fancier name for it like createRedutserCurried, but well, that is also not much better...

@wkrueger wkrueger closed this as completed Aug 2, 2018
@esamattis
Copy link
Author

Yeah, I misunderstood the typing situation. this is actually an any type but vscode was still clever enought to autocomplete it seemingly right so I misunderstood it.

But I played with style of API from scratch and found out that it can be implemented using classes. So I ended up creating Immer Reducer based on it.

@wkrueger
Copy link
Owner

wkrueger commented Dec 7, 2018

Offtopic: I have put redux on soak for the moment. One big issue was the struggle to bring type inference to stateful (classy) connected components.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants