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

Allow custom stores to set custom enhancers (middlewares) #7417

Closed
mitogh opened this issue Jun 20, 2018 · 4 comments
Closed

Allow custom stores to set custom enhancers (middlewares) #7417

mitogh opened this issue Jun 20, 2018 · 4 comments
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Status] Not Implemented Issue/PR we will (likely) not implement.

Comments

@mitogh
Copy link
Member

mitogh commented Jun 20, 2018

Looking into a way to extend the implementation for custom stores, specifically to inject custom middlewares it seems like at the moment this is not possible.

I think this is really helpful mechanism for us (as developers) to decrease complexity from store into middleware or stuff like async actions, as having selctors and resolvers does not fill all scenarios to handle REST actions.

I was looking at #692 however that one is related to the store used internally not for the ones created by third party.

Describe the solution you'd like

I think by adding a new parameter to this function: registerStore as an array for custom enhancers and for registerReducer so this can be set directly when the store is created.

As at the moment the stores only have a single enhancer which is the one used to debug (which is great BTW).

But I think having the opportunity to add custom enhancers here would be great.

Describe alternatives you've considered

Happy to collaborate with a PR for this in order to allow the extension of custom stores as well.

Just wanted to drop the ideas in an issue before moving ahead with that.

Please let me know your thoughts on that one.

Thank you!

@mitogh mitogh changed the title Allow custom stores to set custom enhancers Allow custom stores to set custom enhancers (middlewares) Jun 20, 2018
@aduth
Copy link
Member

aduth commented Jun 20, 2018

For simple middleware usage, you may consider referencing the editor store which has a pattern for extending the store with middlewares:

https://github.com/WordPress/gutenberg/blob/master/editor/store/middlewares.js

@mitogh
Copy link
Member Author

mitogh commented Jun 20, 2018

Thanks for your quick response. I was looking into that example.

However I think that requires for you to define applyMiddlewares and introduce more boilerplate code in your application (or blocks), instead of relaying on one that has been already tested and available via redux package, which might decrease the error rate when you create your own applyMiddlewares, and introduces the risk for your definition to get outdated with the latest version of redux for instance.

Does not have a latest "fix" that is present on latest version of redux.

I feel thinks would be more clear if we have something like:

import { applyMiddleware } from 'redux';
import { composeWithDevTools } from 'redux-devtools-extension';


const details = {
 reducer( state = {}, action ){}
 actions: {}
}

const composeEnhancers = composeWithDevTools({
});

const store = registerStore( STORE_NAME, details, composeEnhancers(
  applyMiddleware(...middleware),
  // other store enhancers if any
) );

Specially when you have to deal with couple of stores. Also this attaches a bit more with the default definition of createStore

@aduth
Copy link
Member

aduth commented Aug 3, 2018

After some back-and-forth in #8341 and in evaluating Redux framework-level explorations at reduxjs/redux#1702, I'm inclined to close this as "wontfix". With the Redux pull request, I think it's made evident that anything which could be achieved as an enhancer or middleware could be equally implemented on a simple base store exposing only dispatch and getState, which is already available on the return value of our registerStore.

To your point of boilerplate, I don't think this prevents us or anyone else from implementing a more durable applyMiddleware which accepts the store object.

In fact, I'm inclined to think it could be as simple as:

const reducer = ( state = {}, action ) => state;
const details = { reducer };
const store = registerStore( STORE_NAME, details );
Object.assign( store, composeEnhancers(
  applyMiddleware( ...middleware ),
  // other store enhancers if any
)( () => store ) );

Let me know what you think. I'm happy to reopen this should be it shown that the current mechanisms are insufficient.

@aduth aduth closed this as completed Aug 3, 2018
@aduth aduth added the [Status] Not Implemented Issue/PR we will (likely) not implement. label Aug 3, 2018
@mitogh
Copy link
Member Author

mitogh commented Aug 3, 2018

Thanks for the follow up here @aduth, however I think by allowing a third parameter during the store registration will provide a more a couple of benefits instead of the approach that uses Object.assign here are some of them

  1. Consistency, as redux createStore takes 3 arguments, by taking the same amount of arguments with registerStore makes it easier to follow any tutorial or resource out there related with redux as createStore has a signature like createStore(reducer, [preloadedState], [enhancer])

  2. Documentation, by following a common pattern and following point above we can rely on any external documentation on redux instead of having to write a specifc one for the type in how gutenberg approach the stores, it also provides a more fast learning curve for people familir with the redux library.

Overall I think there are ways to work around and apply custom enhancers to the registerStore but I think using a common pattern out there will help with the adoption and overall grasp of how things are working, specially because at this point is hard to find docs about how things should work on this.

Thanks again for the follow up here, and let me know if you have any questions. I think closing this one for now is good as you guys might be focusing on higher priority stuff so keep on going 🎉 great job so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Status] Not Implemented Issue/PR we will (likely) not implement.
Projects
None yet
Development

No branches or pull requests

3 participants