State / CPT: Create notices middleware for observing actions

[aduth]

Related: #6536
Related: #6306

This pull request seeks to explore a new middleware for notices to enable monitoring action types and dispatching notice creation in response to those actions. The advantage of this approach over earlier iterations (#6306) is that in using the reducer for observing actions, we don't have access to the entire global state. This makes it difficult to customize the notice, for example including the title of a post when a delete fails, since the action only contains the post and site IDs.

Since the middleware signature allows access to the entire global state, we can make use of any existing selector to retrieve data outside of the action or notices state. It also simplifies and removes redundancy in creating the notices by reusing existing action creators and the reducer behaviors corresponding to those action types.

Testing instructions:

Repeat testing instructions from #6536

Follow-up tasks:

I still think it'd be worth exploring expanding this middleware to capture meta directly from the action, as mentioned at #6306 (comment) .

/cc @mtias @gwwar @artpi

Test live: https://calypso.live/?branch=add/notices-middleware

[gwwar]

What happens when we wish to have different notices that are keyed to the same actions? For example a failed delete in a post list, versus a failed delete in the Calypso editor?

[aduth]

For example a failed delete in a post list, versus a failed delete in the Calypso editor?

Not sure this is something we'd want to do in practice, though it should be do-able; you could, for example, switch on state.ui.section.

[gwwar]

It'd be uncommon, but it's nice to have the flexibility to do so.

[dmsnell]

This is the question I raised in #6262 - it seems like the burden of proof should like in asking, "when would we have a legitimate reason to have different notices for the same type of event."

[gwwar]

The code here looks good. 👍 Having access to the full state tree is a good improvement.

I still think it'd be worth exploring expanding this middleware to capture meta directly from the action

I'm curious to see what this looks like. If we run with this middleware file, it'll grow in size pretty quickly and the notice logic will remain apart from its related sub-tree.

[aduth]

I'm curious to see what this looks like.

In my mind, one could include a meta.notice property on any action which mirrors the structure of the notice object created in createNotice:

https://github.com/Automattic/wp-calypso/blob/eb4252e/client/state/notices/actions.js#L22-L30

I don't know when we'd ever want to use it in this way, so likely best to wait until the need arises.

If we run with this middleware file, it'll grow in size pretty quickly and the notice logic will remain apart from its related sub-tree.

I might argue this is a good thing. Same as in #6306, I quite like that there's a single file where one can observe the different action types which can trigger a notice. Tying to the original action creator also limits us to the same ceveats around global state. I suppose technically one could claim there's getState argument on the thunk signature, but there's a general consensus that this should be avoided.

I considered a "one-per-file" action type to notices behaviors, but quickly ran into the issue that the value of the action type could technically be different than its type name, making it difficult to name files statically. e.g. What if we were to start namespacing the values of our action types with a wpcalypso/ prefix if/when we introduce the idea of plugins?

[aduth]

Pushed a rebase which simplified observers by removing array declaration (premature). Also added tests and moved existing notices/reducer.js handlers to the new observer object. In the process ditched "count"*, but I think we'll not want this anyways, since we should have granular control from the notice to e.g. undo a trash action for an individual post. This will be implemented in a subsequent pull request.

* Not that it's difficult to implement. In fact, here's a refactor of createNotice to allow arbitrary options to be stored in a notice:

diff --git a/client/state/notices/actions.js b/client/state/notices/actions.js
index b75aeb1..c0d4810 100644
--- a/client/state/notices/actions.js
+++ b/client/state/notices/actions.js
@@ -19,19 +19,22 @@ export function removeNotice( noticeId ) {
 }

 export function createNotice( status, text, options = {} ) {
-       const notice = {
-               noticeId: options.id || uniqueId(),
-               duration: options.duration,
-               showDismiss: ( typeof options.showDismiss === 'boolean' ? options.showDismiss : true ),
-               isPersistent: options.isPersistent || false,
-               displayOnNextPage: options.displayOnNextPage || false,
-               status: status,
-               text: text
-       };
+       const {
+               id: noticeId = uniqueId(),
+               showDismiss = true,
+               isPersistent = false,
+               displayOnNextPage = false
+       } = options;

        return {
                type: NOTICE_CREATE,
-               notice: notice
+               notice: {
+                       ...options,
+                       noticeId,
+                       showDismiss,
+                       isPersistent,
+                       displayOnNextPage
+               }
        };
 }

[blowery]

so this file will grow to know about a bunch of different action types?

maybe expose a way to register an observer from the outside? that way action creators and their notices could be bundled together...

[blowery]

ah, now i see the existing discussion in the base ticket..

[aduth]

that way action creators and their notices could be bundled together...

Do you think there's more benefit it keeping them together this way? Or, more generally, do you have concerns about this file growing in size?

[blowery]

a bit. the stuff that's in there now has no bearing on the reader, but it's being included for the reader bundle... it's not that large yet, but if we require all notices to be in here, it'll grow and grow..

[gwwar]

javascript decorators are still stage-1, but if we had them available, I could see us keeping this logic closer together and annotating the class method so that it registers with this middleware

Maybe something very roughly like:

@createNotice( POST_DELETE_SUCCESS )
postDeleteSuccess( action, dispatch ) {
    dispatch( successNotice( translate( 'Post successfully deleted' ) ) );
}

[aduth]

the stuff that's in there now has no bearing on the reader, but it's being included for the reader bundle

I wonder if this could be an argument against keeping them alongside the action creators. It seems sensible to me that the notices behavior should always be globally aware of action types it's concerned with, not dependent on the action creators file having been loaded beforehand (though presumably in most/all cases the action creator to have triggered the action will most likely have been loaded beforehand anyways).

javascript decorators are still stage-1

I've a love/hate relationship with decorators. They seem like a neat idea, but different enough that I worry it's the type of proposal that won't ever be adopted, and would be painful to rip out of code if we were to start depending on it now. I'd be more comfortable waiting until it at least reaches stage 2.

[gwwar]

I'd be more comfortable waiting until it at least reaches stage 2.

Oh definitely, this was just an example of how we could arrange the code in the future if this ever stabalizes

[artpi]

I have no concern with this growing in size whatsoever.
What concerns me here (and it's the same discussion I've been having with @dmsnell over the connection middleware) is the separation of concerns.
In my opinion, middleware.js should not be concerned with translations/logic specific to some actions.
I do not like this centralized mega-case statements or huge arrays.
For the same reason, I do not like "stylesheet" and "javascript' folder separation in some projects, because I think all component logic should be close to that component. When I'm adding / editing component, I should not be venturing outside that folder too often.

Granted, we did not settle upon correct idea to link these "middleware plugins" with main middleware - as I said, I try to come up with something in the connection middleware efforts.

[dmsnell]

I wonder if this could be an argument against keeping them alongside the action creators

as @artpi mentioned, this is the main difference between our API middleware PRs. I do not think that the reader should care or know anything about notices. why should it? the reader knows that it needs to tell Calypso to delete a post or something like that - end of story. it's now up to Calypso to run that action and respond accordingly. if it should fail, then that failure can and will trigger a notice. what if we're offline? do we need to make the reader know about online/offline status?

if the logic for actually accomplishing the action is spread out we will end up with hard to manage spaghetti, but if we split it out then we can take advantage of code reuse, simplification of the independent functional blocks, and management of global/central policies.

[dmsnell]

javascript decorators are still stage-1

we can already accomplish this goal with some code transformation, something @aduth I think you and I discussed with someone else (or more people too). the cool thing about using these macros (however you want to call them, come they from Babel or sweet.js or anywhere else) is that by keeping them in-syntax, we can make a system that works whether or not the macro is active

const handlePostDeleteSuccess = ( … ) => …
createActionHandler( POST_DELETE_SUCCESS, handlePostDeleteSuccess );

we can write createActionHandler first as a simple JS function, then when we get fed-up or experimental, we can write a macro to transform that at compile-time to whatever the decorator would do.

I still vote to keep these handlers all centralized and in one place, even with the decorators.

[artpi]

In my mind, one could include a meta.notice property on any action which mirrors the structure of the notice object created in createNotice:

I like this a lot! ( and was thinking about writing it myself ), so if you need help - ping me.

[dmsnell]

I'm guessing you prefer to not do this, but many of the Redux libraries tend to use fat-arrow functions and without parenthesis for single argument functions (not sure if this is experience or my personal bias giving me this impression). to me it looks far better than the verbose mixture of function( … ) { return ( next ) => ( action ) => … }

export const noticesMiddleware = ( { dispatch, getState } ) => next => action => {
    …
}

also, this threw me for a loop, because I see different definitions for the middleware API depending on where I look. turns out this has been a controversial thing and although I found several PRs/issues dealing with changing the signature of applyMiddleware, I never found the commits that actually changed it ¯\_(ツ)_/¯

[aduth]

many of the Redux libraries tend to use fat-arrow functions and without parenthesis for single argument functions

I'm not a huge fan of the single argument exception for just that reason - it's an exception. One which must be learned, and I don't find the benefit to outweigh this cost and the cost of losing a common pattern for argument declarations (i.e. delineated by parentheses). I also don't see it being too much an issue to mix the two styles of functions if it helps bring a good balance of readability and compactness.

I'd agree though that it is a documented style of Redux middleware to take the inlined arrow function approach, and so to meet those expectations it would make more sense to rewrite this similar to how you'd suggested.

I do have strong opinions on the readability of arrow functions generally, but we can save that debate for another day 😄

[dmsnell]

Thanks for matching the redux style.

It's funny; I feel like the parentheses for multiple arguments are the exception due to JS syntax not dealing with this well...

const combine = a, b => [ a, b ]

We should have some kind of challenge at the GM: Halo tournament, survivalist race, eating contest, or some other kind of death match to resolve these types of matters 😉

[aduth]

Halo tournament

Ooh, it's on!

[aduth]

@dmsnell : Thanks for the detailed feedback. I've incorporated a number of your suggestions in 810a33a . I've still not moved individual handlers to be separate from the object itself because I couldn't think of a nice pattern that satisfies the worry I mentioned earlier. At this point, it might be a bit premature anyways, and I'd be curious to see what trends arise for notice handler logic. If it's mostly like the dispatchSuccess case and few with extensive logic, it'd probably be reasonable to place them separate.

[dmsnell]

there is nobody I am more intimidated by to leave a comment in a review than you @aduth - your level of quality and productivity is an example to us all. Thanks for interacting with me thus through these discussions. Please know hat even when I write about things for which I hold a strong opinion, I am also simply trying to challenge assumptions about the code and explore if there might be other ways to accomplish them - noting is intended personally. Further, I jumped in here because of how closely it relates to the work that @artpi and myself are doing. I'm excited to see a new specialized middleware come about and to see another developer working on his big problem.

[aduth]

there is nobody I am more intimidated by to leave a comment in a review than you @aduth

Ahh! I hate that you feel this way.

I'll never claim to always have it right, and as often as I disagree, these discussions are always formative for me. Code style is an fascinating topic and I'm very interested in understanding the benefits seen by other perspectives on the matter (ask @mcsf, we've discussed this at length recently 😄 ).

So please do continue to challenge me. As evidenced by how this has evolved from #6306, it's indeed a unique problem with a number of implementation approaches worth exploring.

[aduth]

I've pushed another set of revisions in ca092b5 which hopefully addresses the remaining feedback. Specifically:

• Rename HANDLERS to handlers
• Move handler logic to separate functions
  • Mapping hopefully easier to scan with one-line-per-action-type
  • Testing is performed on the exported handler logic directly
• Returned to passing arguments directly, rather than as object
  • Ordered by most relevant to a handler
• A few comment blocks to help break up the file into sensible chunks
  • Maybe later we pull some of these out into their own files

[blowery]

Ah! So much has happened. To respond to one thing I mentioned in a code diff with @dmsnell ...

I do not think that the reader should care or know anything about notices. why should it? the reader knows that it needs to tell Calypso to delete a post or something like that - end of story.

My concern with having all of the notice responders in one file has less to do with who-knows-what and more to do with bundle size. I agree the "reader" (ie, stuff that lives in client/reader) shouldn't know a darn thing about notices. I think the action creators may know about them, but ideally they don't. So then who does know about them, and how do we decide which bundle they should get pulled into?

With the current layout, these will likely end up in the commons. That's probably ok, but as I mentioned, if one loads the reader and nothing else, code that handles actions that never fire (and can never fire) also loads. This is true here, and for our reducer tree. Both are pulling in everything that could possibly happen from the start. I think this is unnecessary bloat and we're going to want / need a system to patch things into the reducer tree dynamically down the road. Our bundles are already pretty darn large. Making large parts of the app global isn't going to help that concern.

[dmsnell]

more to do with bundle size

this is a concern I haven't thought about @blowery. I suppose it's moot for notices since they are more like global actions anyway, but I see your point on others.

since these would all come in through middleware, it seems reasonable that we could possible bundle up the middleware into individual chunks or groups. we can dynamically load in middleware so maybe when we open My Sites (or when anticipating such a navigation) we could start pulling in other chunks.

there's no reason we couldn't split this middleware into smaller chunks than WP API. We could have postMiddleware, siteMiddleware, etc… though it would obviously take more thought than this to choose how to split it up.

[dmsnell]

@aduth this is great stuff. I feel like those of us working with middleware approaches are searching for solid footing in an unstable place, but I guarantee you this is moving in the right direction and far better than what it is replacing. Thanks for the work, thanks for the diligence!

[blowery]

since these would all come in through middleware, it seems reasonable that we could possible bundle up the middleware into individual chunks or groups. we can dynamically load in middleware so maybe when we open My Sites (or when anticipating such a navigation) we could start pulling in other chunks.

My thought for this particular thing was that the middleware would always be present, but code loaded via chunks could register handlers when they load.

The downside there is that if code that registers a handler gets duped into more than one chunk, we'd have a problem with the "same" code registering multiple times, resulting in multiple notices, maybe.

[dmsnell]

but code loaded via chunks could register handlers when they load

it would be worth getting some numbers on how much code the reducers currently consume and how much they would grow.

I'm assuming that right now we have lots of duplicated code lying around in redux-thunks because of this lack of centrality, and I also assume that the reducers may not grow too big.

I will try and fiddle around with some numbers if I can find them.