MM-24009: remove redux-offline #1396
Conversation
| * getAppReducer - A function that returns the appReducer as defined above. Only used in development to enable hot reloading. | ||
| * clientOptions - An object containing additional options used when configuring the redux store. The following options are available: | ||
| * additionalMiddleware - func | array - Allows for single or multiple additional middleware functions to be passed in from the client side. | ||
| * enableThunk - bool - default = true - If true, include the thunk middleware automatically. If false, thunk must be provided as part of additionalMiddleware. | ||
| */ | ||
| export default function configureServiceStore(preloadedState: any, appReducer: any, userOfflineConfig: any, getAppReducer: any, clientOptions: any) { | ||
| const baseOfflineConfig = Object.assign({}, defaultOfflineConfig, offlineConfig, userOfflineConfig); | ||
| export default function configureServiceStore(preloadedState: any, appReducer: any, persistConfig: any, getAppReducer: any, clientOptions: any): Store { |
There was a problem hiding this comment.
Strictly speaking, this is a breaking change for anyone using mattermost-redux to configure their own store. This isn't one of the areas I think we've really implied backwards compatibility though. If we ever do that, we should make this take named arguments in an object instead.
There was a problem hiding this comment.
Agreed re: breaking change, and that we're not explicitly supporting backwards compatibility here. I'm not really sure we should, either, but open for debate!
src/store/configureStore.dev.ts
Outdated
| }); | ||
| } | ||
|
|
||
| return store; | ||
| } | ||
|
|
||
| function createDevReducer(baseState: any, ...reducers: any) { | ||
| return enableFreezing(createReducer(baseState, ...reducers)); | ||
| return createReducer(baseState, ...reducers); |
There was a problem hiding this comment.
Why move enableFreezing out of here? It looks like you're always calling them together anyway.
There was a problem hiding this comment.
Ooh, good catch -- when I originally removed createOfflineReducer, I started getting exceptions around the store being mutated. So to isolate and fix that, I called them in the order enableFreezing(createOfflineReducer(createDevReducer)) and made sure freezing worked before ripping out createOfflineReducer afterwards. The final delta is unnecessary, though -- and in fact createDevReducer is largely superfluous at this point.
In fact, I'm not entirely convinced it's necessary to distinguish .dev.ts from .prod.ts in the first place, given tree-shaking and what not. Either way, I'll clean up this anomaly.
|
/update-branch |
|
@jgilliam17 This has to be reviewed in the corresponding webapp PR |
hmhealey
added
the
Do Not Merge/Awaiting PR
|
@lieut-data I've added the Awaiting PR label since if this goes in first and someone updates their version of mattermost-redux in the webapp, then it'll break due to the change in parameters. One of the many reasons I'm looking forward to getting rid of mattermost-redux |
|
Re-requesting dev review given the changes. Might be easiest to review with whitespace diff disabled. See mattermost/mattermost-webapp#7648 (comment) for context. |
lieut-data
removed
Do Not Merge/Awaiting PR
lieut-data
added
4: Reviews Complete
|
It doesn't look like this change was pushed to the npm version 5.33.1. I'm still seeing |
Summary
Follow the lead of mattermost/mattermost-mobile#4120 and remove redux-offline. The library was responsible for automatically retrying select requests, but the vast majority of API calls remained unaffected. Removing the library simplifies the surface area of the Redux store and corresponding code. Note that this library is discrete from redux-persist, currently responsible for the magic of
storage.storageand rehydrating previously saved drafts, etc.I've broken this PR into various discrete PRs that are worth reviewing independently. But to summarize the other changes that were tangled with removing this library:
remote-redux-devtoolswithredux-devtools-extension-- the former has support for React Native apps and routing requests through remotedev.io, but none of this is needed for the WebApp now that Mattermost Redux is unused by the mobile app.Ticket Link
Fixes: https://mattermost.atlassian.net/browse/MM-24009
Also, see mattermost/mattermost-webapp#7648