Support User: Add (pure) Redux integration (alternate implementation) #2819
Conversation
jordwest
added
[Status] In Progress
[Feature Group] Support
jordwest
force-pushed
the
add/support-user-redux-alt
branch
5 times, most recently
from
8caae34 to
755480b
Compare
| wpcom = require( 'lib/wp/support' )( wpcom ); | ||
| } | ||
|
|
||
| module.exports = wpcom; |
There was a problem hiding this comment.
Note for review: These changes give the unit tests access to the wpcomSupport functions
jordwest
added
[Status] Needs Review
| * update the wpcom API interceptor accordingly. | ||
| */ | ||
| reduxStore.subscribe( () => { | ||
| const { supportUser, supportToken } = getSupportState( getState() ); |
There was a problem hiding this comment.
Would it be possible to listen to an action rather than subscribing to a store change? This is going to fire often.
There was a problem hiding this comment.
There doesn't seem to be any way with Redux to only subscribe to specific events, however this should only fire once per dispatch - which is just as frequently as each reducer function.
Another option might be to move this to a React component, then use connect() and componentWillReceiveProps() to observe the changes. What do you think?
There was a problem hiding this comment.
Right, I couldn't find any better patterns either. 😄
I guess the weirdness comes from the fact that we need to maintain state in two places. I'm OK with the above, but it'd be good if we could refactor this in the future.
jordwest
force-pushed
the
add/support-user-redux-alt
branch
2 times, most recently
from
2fca7ab to
d00d9cb
Compare
| * error occurs in a wpcom API call, the error is forwarded to the | ||
| * Redux store via an action. This also forces any data refreshes | ||
| * that are required due to the change of user. | ||
| */ |
There was a problem hiding this comment.
Minor thing, but multiline comments that are not jsdoc blocks should use //.
|
I think this is better as an intermediary step in keeping things cleaner, thanks for the alternative approach. |
|
👍 retested this and it works well. Thanks for reworking this! One minor thing that could use some improvement is maybe a quick console.log for your current user status if you've logged in successfully. Currently on login, we see a promise, and on logout, an action: |
gwwar
added
[Status] Ready to Merge
and removed
[Status] Needs Review
jordwest
force-pushed
the
add/support-user-redux-alt
branch
2 times, most recently
from
78fc090 to
5ffe20d
Compare
jordwest
force-pushed
the
add/support-user-redux-alt
branch
from
5ffe20d to
8de4589
Compare
This change adds the Redux actions and reducer for the Support User UI in an upcoming PR.
jordwest
force-pushed
the
add/support-user-redux-alt
branch
from
8de4589 to
c6516a6
Compare
Support User: Add Redux integration
This is an alternative implementation of #2512 based on this discussion. The difference in this branch is that the Redux actions and reducers are kept pure with no side effects. To achieve this, the support token is stored in the Redux store and any change is pushed down to the
wpcomlibrary through an interop module. The interop module also triggers any content refreshes required due to the change of support token.Builds upon: #2368
This change moves storage of the support token to the Redux store. There are no user facing functionality changes in this PR since #2368.
ref p195om-2GO-p2