Notices: Redux everything!

[artpi]

Part of ongoing effort to clean up notices:
#888 #1094 #1105 #1313 #1364

What is going on here?

Well, we want global notices to be in redux store.

But you're doing it wrong!

I'm open fo suggestions :)

Plan for now

• wait until Framework: Introduce top-level Redux state directory with sites example #751 is merged
• Polish and finish this. This will introduce second global notification system and 2 will be active at once.
• in different PRs gradually switch from old notice system to new one implementing more comple use cases
• Phase out old system

TODO

[ ] tests
[ ] dismissing
[ ] delay

[aduth]

I must have squashed it into non-existence in #338, but in some of my earlier attempts to introduce a top-level provider, I used it as an opportunity to reduce some of the repetition here:

diff --git a/client/boot/index.js b/client/boot/index.js
index 99df3f4..ef7428d 100644
--- a/client/boot/index.js
+++ b/client/boot/index.js
@@ -163,13 +163,13 @@ function boot() {

                // Create layout instance with current user prop
                Layout = require( 'layout' );
-               layout = React.render( React.createElement( Layout, {
+               layout = React.createElement( Layout, {
                        user: user,
                        sites: sites,
                        focus: layoutFocus,
                        nuxWelcome: nuxWelcome,
                        translatorInvitation: translatorInvitation
-               } ), document.getElementById( 'wpcom' ) );
+               } );
        } else {
                analytics.setSuperProps( superProps );

@@ -179,12 +179,14 @@ function boot() {
                        LoggedOutLayout = require( 'layout/logged-out' );
                }

-               layout = React.render(
-                       React.createElement( LoggedOutLayout ),
-                       document.getElementById( 'wpcom' )
-               );
+               layout = React.createElement( LoggedOutLayout );
        }

+       layout = React.render(
+               React.createElement( ReduxProvider, { store: reduxStore }, () => layout ),
+               document.getElementById( 'wpcom' )
+       );
+
        debug( 'Main layout rendered.' );

        // If `?sb` or `?sp` are present on the path set the focus of layout

[artpi]

I will get around to that once we decide what to do with global store (do we want singleton?)

[artpi]

Pushing right now :)

[artpi]

Fixed!

[aduth]

Notices seem particularly tied to the UI, so it might make sense to include as a sub-tree of the ui state.

{
    ui: {
        selectedSite: null,
        notices: {}
    }
}

The folder structure could reflect this as well:

client/state/
├── index.js
└── ui/
    ├── reducer.js
    └── notices/
        └── reducer.js

Thoughts? These are things that are yet to be nailed down, but will be good to include as recommendations in #1378. I find that it's nice to be able to compose the state tree this way and to have the folder structure mirror the hierarchy. One downside is that test/ exists as a directory but is not in-fact a key in the tree, so it's not entirely consistent.

[artpi]

@aduth , @mtias
I would really want to discuss this:

Why did I do it like this?

TLDR: I needed it in https://github.com/Automattic/wp-calypso/pull/1393/files#diff-48b5d386d8ed23afbf806dd8b9435d7fR1 .

Notices are the first case that is really making use of actions cross-site, so here's the deal:

In order to SEND a notice, we need to do reduxStore.dispatch . We can:
a) use store reference from context, but we need to pass it down from controller to component that wants to send a notice. Sometimes, this can be deep.
b) wrap the component that wants to send a notice in connect. so in mapDispatchToProps actions will be wrapped in dispatch call.

Both of these options would involve re-writing a bazilion components and connecting EVERYTHING to redux, because everything is sending notices.

So, I used a singleton and global notice helper/factory.

I suspect, there will be a lot of instances that we need to get to the store without components and arbitraly.

SSR

As @mtias told me, this can create issues with SSR because that would globalize someone's store and this would run server-side.

But this is EXACTLY what is happening right now. This module:
https://github.com/Automattic/wp-calypso/blob/master/client/notices/index.js

acts like a singleton for notices.

What can we do?

I'm for singleton for a store. If it creates problems with SSR, maybe we can connect it somehow to rotate different stores between users.
But this will become a problem anyway.

My recomendation: Freeze store for SSR. Keeping store state doesent make sense, because SSR only makes sense for different URL's. We cannot transmit store content between url and serwer, so even why bother?
Lets make store read-only for SSR.

[aduth]

Since server-side rendering is the critical piece that justifies instantiating the store on-the-fly, we should be clear about our needs there.

What sense does keeping an application state make for SSR? How do we match state of store with the url to serve a page with a different state?

Because our goal is to serve the same markup from the server as would be rendered in the final state of the client rendering, the server must provide an instance of a Redux store to be used by the component hierarchy in retrieving data. The worry about "leaking data" is that, since a Node server is long-lived, saving a reference to a singleton instance of a store may result in data being shared between requests. If one user's request to the server causes private information to be added to the store and that store is a singleton instance, another user's request may inadvertently gain access to that information. By creating a new store for each and every request, we ensure that this can never happen.

There are quite a few steps between now and being able to render the same on the server as we do on the client, notably:

• Adopt a library that allows us to create instanced stores (Redux)
• Migrate to a global <App /> component so that we're only rendering a single React tree during the lifecycle of a page
• Reconstruct the route handlers for use in the context of a server, since page.js only works in the browser
• Create a mechanism to describe the data needs for a page or set of components, such that the server can detect when the response is ready to be served (after all data has finished fetching)
• Transmit data from the server to the client to avoid unnecessary/duplicated fetching. Redux makes this quite easy by simply making the value of getState available as a global variable on the server-side, then "rehydrating" on the client by passing the initial state as the second parameter when creating the Redux store instance

So while we have many examples of singleton stores in the codebase currently, these will need to be changed as we move closer to achieving these goals.

a) use store reference from context, but we need to pass it down from controller to component that wants to send a notice. Sometimes, this can be deep.

This is not as difficult as it seems when using React context (different from page.js context in your comment). We'll recommend using react-redux to make it even easier. See #751 (comment) as an example.

Not every single one of this 84 files is a component, but I'm guessing that would mean connecting most of the components in the whole of calypso. I can do that, but do we want to reduxify it so much?

The latter part of this comment would seem to imply that it's difficult or inconvenient to use the Redux store instance, in which case this would be a problem to solve at a framework level and not specifically for notices. Personally, I'd agree that this is not as easy as it was previously when we could simply import the module and start using it. Given the considerations above and the conclusion that this is how it must be, we should think about how to ease the transition for those involved in the project.

[artpi]

Thank you @aduth!
I will start implementing your advices and we'll see how it goes :)

[artpi]

This is the place I would like to have some helper, because implementing this action creators in every place that uses notices would be... verbose.

[mcsf]

I've chatted a little with @artpi and wanted to share some of my thoughts on how to do this. Basically, so much of Redux (namely, pure actions) is about declarative, data-centered code. Also, Redux middleware is a central instrument to do smart automations from inbound data/events. So the idea is to, as much as possible, reuse existing actions (that describe meaningful events, like a post being published — unlike an action like CREATE_NOTICE, which in a sense is an action derived from a real action like PUBLISHED_POST) using actions' meta subtree:

// Somewhere in Calypso, someone dispatches:

{
  type: 'PUBLISHED_POST',
  postId: 42,
  meta: {
    notice: {
      text: 'Your post has been published.',
    // defaults can be omitted:
      status: 'info',
      duration: 0,
      showDismiss: true,
      className: ''
    }
  }
}

// In the dispatcher/middleware pipeline:

const noticesMiddleware = store => next => action {
  next( action );
  if ( noticeWorthy( action ) ) {
    store.dispatch( makeNotice( action ) );
  }
}

// An event is 'notice-worthy' if its action carries `notice` meta or if
// it's an exceptional action that we care about, e.g. errors.
const noticeWorthy = action =>
  typeof action === 'object' && (
      action.meta && action.meta.notice ||
      contains( LIST_OF_SERIOUS_ACTIONS, action.type ) );

const makeNotice = action => ( dispatch, getState ) => {
  const id = ++nextId;
  const { notice } = action.meta;

  dispatch( {
    type: CREATE_NOTICE,
    id,
    action,
    notice
  } );

  if ( notice.duration )
    setTimeout( dispatch( {
      type: DELETE_NOTICE,
      id
    } ), notice.duration );
};


// In the notices reducer:

const reducer = ( state = initialState, action ) => {
  switch ( action.type ) {
    CREATE_NOTICE:
      return {
        ...state,
        [ action.id ]: action.action
      };

    DELETE_NOTICE:
      return omit( state, action.id );
  }
};

[artpi]

@mcsf 's proposition makes A LOT of sense, because it would mean moving control (displaying a notice is a controllers concern) away from view (component).

[aduth]

I've chatted a little with @artpi and wanted to share some of my thoughts on how to do this

@mcsf : I like this idea. It's a departure from how we use notices currently, though I think it's accurate to think of notices as being in response to other actions in the system. Some questions we can think about as we move towards something like this:

• What if I want to publish a post but not trigger a notice? Do we really want to tie data syncing concerns to the UI like this?
• Does it make sense to have a consolidated mapping of action types to notice messages, rather than attaching as meta?

[mcsf]

• What if I want to publish a post but not trigger a notice? Do we really want to tie data syncing concerns to the UI like this?

Hm. I don't have the answer for this yet.

• Does it make sense to have a consolidated mapping of action types to notice messages, rather than attaching as meta?

My initial reaction was to work out something like that, but as I was discussing it in-place with @seear and @ehg (who'd mumbled something about middleware) they suggested going with the one simplest approach, and stating everything in the payload seemed to satisfy that.

[mtias]

it seems you picked up some unrelated changes?

[artpi]

Because I have initially branched of @aduth 'ies branch to work on this, I need to close this PR because the branch is now merged.
Github does not allow for changing target branched and I need to change to master.

This is new PR for these changes:

1496