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

[State Management] State containers and redux middlewares types mismatch #54438

Closed
Dosant opened this issue Jan 10, 2020 · 5 comments
Closed
Labels
bug Fixes for quality problems that affect the customer experience Feature:StateManagement impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort

Comments

@Dosant
Copy link
Contributor

Dosant commented Jan 10, 2020

Kibana version: master

Elasticsearch version:

Server OS version:

Browser version:

Browser OS version:

Original install method (e.g. download page, yum, from source, etc.):

Describe the bug:

Was trying to use redux middlewares together with state containers and had to force cast to to make it work together.

Steps to reproduce:
Try to use redux-logger together with state containers and see the error

yarn add redux-logger @types/redux-logger

 import logger from 'redux-logger'
  const c = createStateContainer({ test: 'test' });
  c.addMiddleware(logger);

error:

TS2345: Argument of type 'Middleware' is not assignable to parameter of type 'Middleware<Readonly<{ test: string; }>>'.   Types of parameters 'api' and 'store' are incompatible.     
Type 'Pick<ReduxLikeStateContainer<Readonly<{ test: string; }>, any, {}>, "getState" | "dispatch">' is not assignable to type 'MiddlewareAPI<Readonly<{ readonly test: string; }>>'.      
Types of property 'dispatch' are incompatible.         
Type '(action: TransitionDescription<string, any[]>) => void' is not assignable to type 'Dispatch<Readonly<{ readonly test: string; }>>'. Types of parameters 'action' and 'action' are incompatible. 
Type 'A' is not assignable to type 'TransitionDescription<string, any[]>'.               Property 'args' is missing in type 'Action' but required in type 'TransitionDescription<string, any[]>'.

I've tried to make args optional on TransitionDescription, but it led to another error about type Dispatch mismatch.

Expected behavior:

stateContainer.addMiddleware() should work with redux middlewares without type casting.

Screenshots (if relevant):

Errors in browser console (if relevant):

Provide logs and/or server output (if relevant):

Any additional context:

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@streamich
Copy link
Contributor

Is it because of the ReadOnly type? I was thinking maybe we should remove deep freeze readonly type from state container types.

@Dosant
Copy link
Contributor Author

Dosant commented Jan 10, 2020

Is it because of the ReadOnly type? I was thinking maybe we should remove deep freeze readonly type from state container types.

Not completely sure if this particular one is because of readonly.

But I agree, that Readonly type makes things a lot more complicated and maybe having a runtime check would be enough ...

@streamich streamich removed their assignment Jan 10, 2020
@streamich
Copy link
Contributor

But I agree, that Readonly type makes things a lot more complicated and maybe having a runtime check would be enough ...

Agreed, in my expressions branch I actually removed the readonly type and left only the runtime check.

Dosant added a commit that referenced this issue Jan 13, 2020
Some maintenance and minor fixes to state containers based on experience while working with them in #53582

Patch unit tests to use current "terminology" (e.g. "transition" vs "mutation")
Fix docs where "store" was used instead of "state container"
Allow to create state container without transition.
Fix freeze function to deeply freeze objects.
Restrict State to BaseState with extends object.
in set() function, make sure the flow goes through dispatch to make sure middleware see this update
Improve type inference for useTransition()
Improve type inference for createStateContainer().

Other issues noticed, but didn't fix in reasonable time:
Can't use addMiddleware without explicit type casting #54438
Transitions and Selectors allow any state, not bind to container's state #54439
Dosant added a commit to Dosant/kibana that referenced this issue Jan 13, 2020
Some maintenance and minor fixes to state containers based on experience while working with them in elastic#53582

Patch unit tests to use current "terminology" (e.g. "transition" vs "mutation")
Fix docs where "store" was used instead of "state container"
Allow to create state container without transition.
Fix freeze function to deeply freeze objects.
Restrict State to BaseState with extends object.
in set() function, make sure the flow goes through dispatch to make sure middleware see this update
Improve type inference for useTransition()
Improve type inference for createStateContainer().

Other issues noticed, but didn't fix in reasonable time:
Can't use addMiddleware without explicit type casting elastic#54438
Transitions and Selectors allow any state, not bind to container's state elastic#54439
Dosant added a commit that referenced this issue Jan 13, 2020
Some maintenance and minor fixes to state containers based on experience while working with them in #53582

Patch unit tests to use current "terminology" (e.g. "transition" vs "mutation")
Fix docs where "store" was used instead of "state container"
Allow to create state container without transition.
Fix freeze function to deeply freeze objects.
Restrict State to BaseState with extends object.
in set() function, make sure the flow goes through dispatch to make sure middleware see this update
Improve type inference for useTransition()
Improve type inference for createStateContainer().

Other issues noticed, but didn't fix in reasonable time:
Can't use addMiddleware without explicit type casting #54438
Transitions and Selectors allow any state, not bind to container's state #54439
chrisronline pushed a commit to chrisronline/kibana that referenced this issue Jan 13, 2020
Some maintenance and minor fixes to state containers based on experience while working with them in elastic#53582

Patch unit tests to use current "terminology" (e.g. "transition" vs "mutation")
Fix docs where "store" was used instead of "state container"
Allow to create state container without transition.
Fix freeze function to deeply freeze objects.
Restrict State to BaseState with extends object.
in set() function, make sure the flow goes through dispatch to make sure middleware see this update
Improve type inference for useTransition()
Improve type inference for createStateContainer().

Other issues noticed, but didn't fix in reasonable time:
Can't use addMiddleware without explicit type casting elastic#54438
Transitions and Selectors allow any state, not bind to container's state elastic#54439
jkelastic pushed a commit to jkelastic/kibana that referenced this issue Jan 17, 2020
Some maintenance and minor fixes to state containers based on experience while working with them in elastic#53582

Patch unit tests to use current "terminology" (e.g. "transition" vs "mutation")
Fix docs where "store" was used instead of "state container"
Allow to create state container without transition.
Fix freeze function to deeply freeze objects.
Restrict State to BaseState with extends object.
in set() function, make sure the flow goes through dispatch to make sure middleware see this update
Improve type inference for useTransition()
Improve type inference for createStateContainer().

Other issues noticed, but didn't fix in reasonable time:
Can't use addMiddleware without explicit type casting elastic#54438
Transitions and Selectors allow any state, not bind to container's state elastic#54439
@Dosant Dosant added bug Fixes for quality problems that affect the customer experience impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels May 28, 2020
@Dosant Dosant added the triaged label Jun 12, 2020
@exalate-issue-sync exalate-issue-sync bot added loe:medium Medium Level of Effort and removed loe:small Small Level of Effort labels Jun 21, 2021
@ppisljar
Copy link
Member

ppisljar commented Aug 8, 2022

Thank you for contributing to this issue, however, we are closing this issue due to inactivity as part of a backlog grooming effort. If you believe this feature/bug should still be considered, please reopen with a comment.

@ppisljar ppisljar closed this as completed Aug 8, 2022
kibana-app-arch automation moved this from To triage to Done in current release Aug 8, 2022
@exalate-issue-sync exalate-issue-sync bot added loe:small Small Level of Effort and removed loe:medium Medium Level of Effort labels Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:StateManagement impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort
Projects
kibana-app-arch
  
Done in current release
Development

No branches or pull requests

4 participants