Directly: Add Redux interface to imperative API #11490

mattwondra · 2017-02-20T22:35:51Z

Directly is a third-party customer support tool/widget. The Directly API is abstracted away in #11489. This PR adds a Redux interface to this API, so that it can be integrated into the React UI in #11491.

This should read as a fairly straightforward Redux implementation of the available API actions. The Directly API is largely a black box where we can fire events in but there are no callbacks or listeners we can register. So the state transitions and information are pretty basic.

We're also only keeping track of the state we may need and can trust. For example we could keep an isMinimized flag that toggles with the appropriate Redux actions, however the user can minimize/maximize Directly using in-widget controls without any way for us to be notified.

These Redux actions are not yet integrated into any UI, so this PR should have no side effects.

matticbot · 2017-02-20T22:35:53Z

Test live: https://calypso.live/?branch=add/directly-redux

jordwest · 2017-02-21T10:04:35Z

client/state/action-types.js

+export const DIRECTLY_ASK_QUESTION = 'DIRECTLY_ASK_QUESTION';
+export const DIRECTLY_INITIALIZE = 'DIRECTLY_INITIALIZE';
+export const DIRECTLY_MAXIMIZE = 'DIRECTLY_MAXIMIZE';
+export const DIRECTLY_MINIMIZE = 'DIRECTLY_MINIMIZE';


~~How do these actions get called if the user clicks the minimize button on the widget? Is that button under our control or would we need an event listener to detect when that happens?~~

Oops, I just reread the description :) You mentioned we're not keeping track of the state so there's no need for these to be called when the widget button is clicked.

If that's the case, do we need these actions? Is there any plan for the Calypso UI to trigger them, or could we simply leave them out until/if needed?

Right — we have no way to hook into widget-initiated events, so these actions are not being tracked in state.

Currently there's no plan for MAXIMIZE, MINIMIZE, or OPEN_ASK_FORM. (I could see us using min/max as we go through some UX testing though). I figured I'd include them because this is actually the entire Directly API, so it saves future devs from having to dive into Directly's docs. But if it's better to simply eliminate them for now, I'm amenable to that.

Might it also be best to remove them from the lib/directly API?

jordwest · 2017-02-21T10:14:16Z

client/state/directly/actions.js

+export const askQuestion = ( questionText, name, email ) => ( dispatch ) => {
+	dispatch( { type: DIRECTLY_ASK_QUESTION, questionText, name, email } );
+	askDirectlyQuestion( questionText, name, email );
+};


Redux middleware might be more suitable for these side-effecting actions. That way the action creators can return plain objects without any unexpected effects, while the directly methods get called only when the action is dispatched.

A few examples:

Automated Transfer - clears the browser storage and refreshes the page

Audio - plays sound when an action is dispatched

Notices

cc @dmsnell

These started out in middleware, but at the time directly.initialize() was an async function that took a callback and dispatched a new success or failure action. I was advised to move them to Action Creators to avoid dispatching new actions within middleware.

So just confirming — now that the Directly functions are fully synchronous and we're not responding to callbacks with more actions, is middleware the best place for these? And if we were to modify them to accept callbacks for success/failure, should they be moved back out of middleware into action creators?

/cc @omarjackman

I was advised to move them to Action Creators to avoid dispatching new actions within middleware.

I'm not sure where you got this information but unfortunately something got lost in translation. The middleware API is explicitly given access to the dispatch() method of the store plus it's given next() as a way of passing an action along the middleware chain.

As @jordwest indicated any side-effecting operation should be moved into the middleware and away from the actions themselves.

I'd be happy to discuss this further with you and share about the operation and role of middleware, if you would like.

Ok — so is there a rule of thumb for when to use middleware instead of a thunk from an action creator for side-effecting operations? Many action creators initiate side-effects in their returned thunk functions (e.g. savePost) so what exactly makes that case different from my Directly side-effecting operations?

I think intuitively middleware feels like the right place to me too, but I could convince myself either way so I'd love some more concrete principles to ground myself on.

I'm not sure where you got this information but unfortunately something got lost in translation

@dmsnell That info came from me. I started out by saying lets do it in middleware then it seemed like it'd be better suited for the action creators because I've seen some hairy situations where actions get dispatched from middleware and cause problems. either way, my bad 😄

Ok — so is there a rule of thumb for when to use middleware instead of a thunk from an action creator for side-effecting operations?

why yes there is! always use middleware and never use action thunks 😄

not kidding. if it's confusing please ask for help - we'll happily walk you through the process!

I've seen some hairy situations where actions get dispatched from middleware and cause problems

sorry to hear that @omarjackman - this probably reveals issues with the logic more than the use of middleware. I'd be happy to help you sort those out if you want to revisit the code. thunks introduce unnecessary complexity and opaqueness to the otherwise elegant Redux pattern and they are a tempting but nefarious work-around to the system.

always use middleware and never use action thunks

Interesting, I didn't realize action thunks were discouraged

well truth be told I've carried the banner for the fight against them but that's because of the damage they do to our ability to introspect, modify, and centrally manage our app policies and behaviors. they are convenient for local concerns but since they fly through the dispatcher hiding what could be any code imaginable they block our ability to perform meta operations on the actions, orient our code to component-specific changes instead of app-specific intentions, and present a mess for maintaining the hundreds of related thunk action types that exist in the Calypso code nebula

mtias · 2017-02-21T14:56:33Z

client/state/index.js

@@ -71,6 +72,7 @@ export const reducer = combineReducers( {
 	componentsUsageStats,
 	countryStates,
 	currentUser,
+	directly,


What about putting this inside support/directly or help/directly? It's not immediately clear what it is about.

Sure, nesting this makes sense. Directly is similar to Olark and Happychat in purpose (live chat support) and I see those are both in ui — does ui/directly sound reasonable?

I wouldn't put it in ui/directly using ui/olark as a reference. Some of the stuff in ui/olark is in ~~the wrong~~ a less than ideal place and will be ripped out later this month.

dmsnell · 2017-02-22T01:34:26Z

client/state/directly/reducer.js

+
+export const isInitialized = createReducer( false, {
+	[ DIRECTLY_INITIALIZE ]: () => true,
+} );


there's probably no actual need to track an isInitialized state. it might be the case but probably isn't. for one, we can represent the initialization state as the presence or absence of other data which we care about; second, do we need to directly show if it has initialized yet to the user?

I like to challenge it whenever I see these kinds of boolean state values.

further, we don't really have a need to make such heavy reducer functions when the logic is actually primitive:

export config = ( state = null, { type, config } ) => DIRECTY_INITIALIZE === type ? config : state;

Right now, there's no use case for isInitialized at all so I'm very comfortable removing it. In that respect, there's also not really a use case for tracking config at all — should I remove that as well and if we find a need for it later we can re-introduce it?

And re: createReducer being heavy — should the default stance be not to use this helper without solid reasoning (vs. using it always to keep reducer markup consistent)?

@aduth should chime in here since he's the wizard behind it.

I personally don't use it and think we should find a burden of need before using it because it does introduce hidden overhead. It's very good for when we need memoized reducers for performance-sake, but that's rarer than it seems.

we must remember that every single reducer runs on every single Redux action dispatch…

I'm very comfortable removing it. In that respect, there's also not really a use case for tracking config at all — should I remove that as well

please! the Redux store should hold the minimum amount of data needed to render the view. anything that can be calculated or derived from more primitive/base data should probably be done so on a function running a render time. that will keep our state smaller and faster due to having fewer reducers and actual state.

@aduth should chime in here since he's the wizard behind it.

Nope 😛 (Originally #4805)

It's a little unfair to claim that the utility creates heavy reducers though; by it's implementation, it's effectively a hasOwnProperty check for an unhandled action type.

The main advantage I see, aside from compactness, is turning state persistence into an opt-in than opt-out by default.

It's a little unfair to claim that the utility creates heavy reducers though; by it's implementation, it's effectively a hasOwnProperty check for an unhandled action type.

thanks @aduth - I think I've had the wrong impression of it. still, I find it obscure because I never can remember what all it does.

marginally related: @gwwar and I have spoken about the idea of making persistence opt-in only and with schema validation. the idea is that we could add some .schema property on a reducer if we want it to persist and without that it just wouldn't save.

Yeah it's a weakness of the first implementation that we persist by default if we use a vanilla reducer.

Whatever helpers we use, opt-in only for persistence is much more dev friendly. Folks might not know about persistence, and we avoid future data shape/bad app state bugs.

dmsnell · 2017-02-22T01:35:54Z

client/state/selectors/get-directly-config.js

+ * @return {Object} The configuration options used by Directly
+ */
+export default function getDirectlyConfig( state ) {
+	return state.directly.config;


we can be sensitive to the fact that this is an unsafe operation when directly doesn't exist. using a safe-defaulting getter could remove the risk…

export const getDirectlyConfig = state => get( state, 'directly.config' );

dmsnell · 2017-02-22T22:43:28Z

nice changes on the API @mattwondra - I might recommend considering the API middleware in state/data-layer (check out the README). we have a flexible way of introducing middleware handlers that will only trigger on specific actions. since I don't really understand what directly is or what its doing for us I can't comment too much; in fact it would be really helpful in the state tree folder to have a README describing why we use it, what it does, the issues with not getting all updates from it, and a guide/walkthrough-style explanation of its role in the codebase.

mattwondra · 2017-02-23T13:39:12Z

@dmsnell Thanks, I'll check out stata/data-layer.

Directly itself is better-documented in lib/directly (see the PR this one depends on). I'll take a little time today to spruce that README up, and I'll make obvious links to it from READMEs in other sub-dirs related to Directly. Does that work?

mattwondra · 2017-02-23T18:42:16Z

@dmsnell I moved custom middleware to state/data-layer — the only think I'm not clear on is file structure, since Directly is kind of a one-off third-party API. I went with state/data-layer/third-party/directly but if you have other suggestions please let me know.

dmsnell · 2017-02-23T21:30:03Z

Does that work?

yep.

I went with state/data-layer/third-party/directly but if you have other suggestions please let me know.

that sounds like a great choice. as the data layer became a thing and raised our awareness of middleware it kind of expanded past its original scope into a way to create additional properly-separated middleware handlers without introducing much overhead.

@aduth do you concur on this? I've been thinking that at some point soon I should rename some pieces in there and update the documentation to suggest as much…

aduth · 2017-02-24T02:10:21Z

Seems fine to me; do we need third-party as a nested directory though? Seems to imply that wpcom is somehow "native", but my understanding of data-layer is that there's not a bias toward any one particular API implementation.

dmsnell · 2017-02-24T02:42:33Z

Seems fine to me; do we need third-party as a nested directory though? Seems to imply that wpcom is somehow "native", but my understanding of data-layer is that there's not a bias toward any one particular API implementation.

I kind of like the idea since this is fringe code. I want to preserve the 1-to-1 mapping between file folder structure and the WordPress.com API path but this (not being our API) doesn't fit that paradigm. I don't know the answer…again largely because the data layer has grown (and I think appropriately so) from what I had in mind when originally creating it.

matticbot · 2017-02-28T20:40:30Z

@mattwondra This PR needs a rebase

Update Directly Redux integration based on feedback - Move side-effects from thunks to middleware - Remove unused state and associated selectors Namespace Directly state to /help/ Use newer data-layer for side-effects instead of custom middleware Remove unused Directly actions Support changes from underlying Directly library

Re-introduces the work done in #11490 which was reverted in #11659. The underlying bugs that caused the reversion have already been fixed in #11684.

Recently the data layer experienced a total failure when #11490 merged in and crashed when it failed to initialize the new middleware handler. The fact that a key was requested for which no value existed led to `config()` raising an `Error` which causes the entire middleware chain to fail to build. Now `config()` has been modified to only raise an actual `Error` when the `NODE_ENV=developement` and instead in all other environments to simply log a message to the browser console and return `undefined`. This should allow us to still catch any mistakes by the obvious message while preventing such otherwise-small errors from cascading out-of-control.

* config(): Add safety falback for when running in production Recently the data layer experienced a total failure when #11490 merged in and crashed when it failed to initialize the new middleware handler. The fact that a key was requested for which no value existed led to `config()` raising an `Error` which causes the entire middleware chain to fail to build. Now `config()` has been modified to only raise an actual `Error` when the `NODE_ENV=developement` and instead in all other environments to simply log a message to the browser console and return `undefined`. This should allow us to still catch any mistakes by the obvious message while preventing such otherwise-small errors from cascading out-of-control.

Re-introduces the work done in #11490 which was reverted in #11659. The underlying bugs that caused the reversion have already been fixed in #11684.

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 20, 2017

mattwondra changed the title ~~Directly: Add Redux interface to imperative Directly API~~ Directly: Add Redux interface to imperative API Feb 20, 2017

mattwondra mentioned this pull request Feb 20, 2017

Contact: Add Directly integration #11306

Closed

mattwondra added [Feature Group] Support All things related to WordPress.com customer support. [Status] In Progress labels Feb 20, 2017

This was referenced Feb 21, 2017

Directly: Add library that wraps around standard API #11489

Merged

Directly: Integrate RTM widget with Contact Us form #11491

Merged

mattwondra added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 21, 2017

mattwondra requested review from omarjackman, jordwest and southp February 21, 2017 04:02

jordwest reviewed Feb 21, 2017

View reviewed changes

mtias reviewed Feb 21, 2017

View reviewed changes

dmsnell reviewed Feb 22, 2017

View reviewed changes

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 22, 2017

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 23, 2017

mattwondra force-pushed the add/directly-redux branch from ae3b20a to ff7bcb9 Compare February 23, 2017 19:59

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 23, 2017

mattwondra force-pushed the add/directly-library branch from b15d167 to a29ead2 Compare February 24, 2017 19:32

mattwondra force-pushed the add/directly-redux branch from ff7bcb9 to fbe098d Compare February 24, 2017 19:40

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 24, 2017

mattwondra force-pushed the add/directly-redux branch from fbe098d to c3b52b2 Compare February 24, 2017 20:42

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 24, 2017

mattwondra force-pushed the add/directly-library branch from bea9610 to 5142e5d Compare February 24, 2017 20:59

mattwondra force-pushed the add/directly-redux branch from c3b52b2 to 8455bcf Compare February 24, 2017 21:02

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 24, 2017

mattwondra force-pushed the add/directly-library branch from 765ec76 to d0024ab Compare February 28, 2017 16:58

mattwondra force-pushed the add/directly-redux branch from 8455bcf to b1a61bb Compare February 28, 2017 18:05

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 28, 2017

mattwondra force-pushed the add/directly-library branch from 9bbb501 to a9caf58 Compare February 28, 2017 19:39

matticbot added the [Status] Needs Rebase label Feb 28, 2017

mattwondra force-pushed the add/directly-redux branch from b1a61bb to 0ee419e Compare February 28, 2017 20:53

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Feb 28, 2017

mattwondra changed the base branch from add/directly-library to master February 28, 2017 20:53

mattwondra merged commit 0a5e439 into master Feb 28, 2017

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 28, 2017

gwwar mentioned this pull request Mar 1, 2017

Revert "Directly: Add Redux interface to imperative API" #11659

Merged

This was referenced Mar 1, 2017

Directly: Add API integration (take 2) #11677

Closed

Directly: Add API integration (take 2) #11684

Merged

mattwondra added a commit that referenced this pull request Mar 1, 2017

Cherry pick Directly Redux integration code from reverted PR

4eba4c5

Re-introduces the work done in #11490 which was reverted in #11659. The underlying bugs that caused the reversion have already been fixed in #11684.

mattwondra mentioned this pull request Mar 1, 2017

Directly: Add Redux interface (take 2) #11685

Merged

dmsnell mentioned this pull request Mar 1, 2017

config(): Add safety falback for when running in production #11686

Merged

mattwondra added a commit that referenced this pull request Mar 6, 2017

Cherry pick Directly Redux integration code from reverted PR

c12dcc1

Re-introduces the work done in #11490 which was reverted in #11659. The underlying bugs that caused the reversion have already been fixed in #11684.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Directly: Add Redux interface to imperative API #11490

Directly: Add Redux interface to imperative API #11490

mattwondra commented Feb 20, 2017 •

edited

Loading

matticbot commented Feb 20, 2017

jordwest Feb 21, 2017

mattwondra Feb 21, 2017

jordwest Feb 21, 2017

mattwondra Feb 21, 2017

dmsnell Feb 22, 2017

mattwondra Feb 22, 2017

omarjackman Feb 22, 2017 •

edited

Loading

dmsnell Feb 22, 2017

omarjackman Feb 22, 2017

dmsnell Feb 22, 2017

mtias Feb 21, 2017

mattwondra Feb 21, 2017

omarjackman Feb 22, 2017

dmsnell Feb 22, 2017

mattwondra Feb 22, 2017

dmsnell Feb 22, 2017

dmsnell Feb 22, 2017

aduth Feb 22, 2017

dmsnell Feb 22, 2017

gwwar Feb 22, 2017

dmsnell Feb 22, 2017

dmsnell commented Feb 22, 2017

mattwondra commented Feb 23, 2017

mattwondra commented Feb 23, 2017

dmsnell commented Feb 23, 2017

aduth commented Feb 24, 2017

dmsnell commented Feb 24, 2017

matticbot commented Feb 28, 2017

Directly: Add Redux interface to imperative API #11490

Directly: Add Redux interface to imperative API #11490

Conversation

mattwondra commented Feb 20, 2017 • edited Loading

matticbot commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omarjackman Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell commented Feb 22, 2017

mattwondra commented Feb 23, 2017

mattwondra commented Feb 23, 2017

dmsnell commented Feb 23, 2017

aduth commented Feb 24, 2017

dmsnell commented Feb 24, 2017

matticbot commented Feb 28, 2017

mattwondra commented Feb 20, 2017 •

edited

Loading

omarjackman Feb 22, 2017 •

edited

Loading