Sharing: Implement Redux-based Publicize reducers and actions #338

aduth · 2015-11-20T20:55:52Z

Closes #350
Partially implements #129

This pull request seeks to implement a new set of Redux-based actions, reducers, selectors, and components related to displaying active Publicize connections in the post editor Sharing accordion. This will serve to eliminate an unnecessary request to the REST API for retrieving Keyring connections, as this data is not currently in use by the post editor.

Implementation notes:

This is a large pull request, but it is mostly comprised of new modules, of which include significant non-front-facing changes (i.e. documentation, testing).

As this pull request introduces Redux, it also includes the necessary changes to wire a new instance of a global Redux store to the routing context. See shared/lib/redux-store . The intention is that as reducers are added, they should be included in the combineReducers call in shared/lib/redux-store/reducers.js . Similarly, as new actions are introduced, they would be added to shared/lib/redux-store/action-types.js . The proposed advantage here is that, by having a single set of action type keys, there's a reduced chance for key conflicts and that this list will serve as a single reference for all action types in use in the application. (See #338 (diff))

react-redux is used here for passing the store context through the hierarchy of post editor components. This is used by a new <EditorSharingContainer /> component for managing and fetching site connections.

A nested subdirectory was created for Publicize-related files, as it's intended that we'll soon introduce new reducers and actions for managing Keyring connections.

Testing instructions:

The changes herein replace the current post editor Sharing accordion connections listing. Verify that existing behavior remains unaffected.

Navigate to the Calypso post editor
Select a site, if prompted
Expand the sidebar Sharing accordion
Note that connections available to the current user (shared or owned) are listed
Click Connect New Service
Connect or disconnect to a Publicize service
Close the popup window after having completed step 6
Note that connections are refreshed and list connections available to the current user

jkudish · 2015-11-21T03:12:40Z

Testing this: the sharing accordion flashes on the page and then disappears. I only had a few online minutes (while travelling) to test, so I haven't yet debugged to find the cause. I did confirm that the sites/$site/publicize-connections request was fired off and returned the correct 3 connections for the site I was testing with. The site in question was a Jetpack site.

The problem didn't happen on a different (WP.com) site.

jkudish · 2015-11-21T05:20:33Z

package.json

@@ -70,7 +70,10 @@
    "raf": "2.0.4",
    "react": "0.13.3",
    "react-day-picker": "1.1.0",
+    "react-redux": "3.1.0",


I'm guessing you didn't go with 4.0.0 because we're still on react 0.13.x?

I'm guessing you didn't go with 4.0.0 because we're still on react 0.13.x?

Yep, react-redux 4.x requires React 0.14. The biggest change between react-redux 3.x and 4.x that we'll need to account for is that the latter does not require the <Provider> child to be a callback function.

https://github.com/rackt/react-redux/releases/tag/v4.0.0

jkudish · 2015-11-21T06:12:35Z

Testing this: the sharing accordion flashes on the page and then disappears. I only had a few online minutes (while travelling) to test, so I haven't yet debugged to find the cause. I did confirm that the sites/$site/publicize-connections request was fired off and returned the correct 3 connections for the site I was testing with. The site in question was a Jetpack site.

Confirmed this problem doesn't exist in master for the same site.

jkudish · 2015-11-21T06:24:27Z

Testing some more, I found that the Sharing accordion would eventually appear, and then I quickly tracked it down to not appearing until a post was initialized (in the case of a brand new post, it didn't initialize until I made a change)

jkudish · 2015-11-21T06:25:18Z

Other than the post initialization bug I found, this is working pretty well for me. Nice work 👍

I'd suggest more 👀 on here, especially since this is my first exposure to redux, personally.

jkudish · 2015-11-21T06:27:28Z

client/post-editor/editor-sharing/publicize-services.jsx

+		const currentUser = user.get();
+		const connections = currentUser ? this.props.connections.filter( ( connection ) => {
+			const { service, keyring_connection_user_ID, shared } = connection;
+			return service === serviceName && ( shared || keyring_connection_user_ID === currentUser.ID );


While I am not sure that a scenario exists where a publicize connection could have a user_ID different from keyring_connection_user_ID, and not be a shared connection, I think that connection.user_ID === currentUser.ID would also be a valid condition here.

aduth · 2015-11-22T21:04:03Z

Other than the post initialization bug I found, this is working pretty well for me. Nice work 👍

I observed and fixed a few bugs related to changing routes after having visited the editor. I'm not able to reproduce the original issue. Can you take another look and let me know if you're still having trouble?

nylen · 2015-11-23T21:42:50Z

client/post-editor/editor-sharing/container.jsx

+	( state ) => {
+		return { state };
+	}
+)( EditorSharingContainer );


Is the state mapping function always going to be the same? If not, why not (for my own education)? If so, it looks like this will be a common pattern going forward, so I think it makes sense to move this logic into a helper method like reduxify or similar.

I'm also finding it pretty confusing that state is used to refer to the state of the Redux store - this.props.state is a strange thing to see in the code. Perhaps calling it store or reduxState or something else would help?

Is the state mapping function always going to be the same?

This raises a good point, as it should depend on the data needs of the component. The react-redux docs examples may help to shed some light here. In this case, rather than injecting the entire state tree, I should probably leverage the existing selectors, specifying connections and hasFetchedConnections as the props passed by the connect mapping function.

I'm also finding it pretty confusing that state is used to refer to the state of the Redux store - this.props.state is a strange thing to see in the code.

"State" is the terminology used by Redux, though I do agree that it becomes a bit confusing in this example where I'm injecting a state prop into the the component. In consideration of the above, this will likely not be as big a concern if I choose to not pass the entire state tree in to the component, and instead pass only the subset of data needed by the component.

Ah, now I remember why it wasn't simple to pass the subset of data -- since the selected site is not yet made available in the state tree, it's not possible to call hasFetchedConnections and getConnectionsBySiteId from the connect mapping function.

Agreed, state maybe should be a reserved keyword

It appears that there is an alternate function signature for the connect mapping function, where the second parameter passes the props of the component, which should include site. Will see if I can use this to select based on the passed site prop.

nylen · 2015-11-23T21:57:05Z

This works well according to my testing. In particular, I was not able to reproduce the issue Joey noticed.

This is my first exposure to Redux as well, but from reading the code and documentation this makes sense at a high level. It'll be nice to have this cleaner implementation of our other data stores too - SitesList in particular comes to mind as code that is not very pleasant to work with.

aduth · 2015-11-23T22:03:22Z

I rewrote the connect mapping function in 7102a9a to only pass the subset of data used by the component. I find this to be much cleaner, avoids this.props.state within the component, and overall seems more in-line with the intended use of react-redux's connect function.

jkudish · 2015-11-23T22:06:16Z

I am still observing the bug I initially described with some sites only.

Resolves the case where if the site were to change while the component is still mounted, a connection fetch would not be triggered if those connections had not already been fetched.

Can occur when navigating from a site-specific page to a non-site-specific page, during which time the selected site is unset while the component is still mounted.

jkudish · 2015-11-24T19:10:02Z

Re-tested and tried my best to break things. All is good.

Sharing: Implement Redux-based Publicize reducers and actions

aduth · 2015-11-24T19:19:53Z

Thanks for all of the great feedback, @jkudish , @mcsf , and @nylen . I'm excited to start using Redux 😄

aduth self-assigned this Nov 20, 2015

kriskarkoski mentioned this pull request Nov 20, 2015

Editor: no need to fetch keyring connections #350

Closed

aduth added the Framework label Nov 20, 2015

jkudish reviewed Nov 21, 2015
View reviewed changes

aduth mentioned this pull request Nov 23, 2015

Turn Flux Action Creators into Redux ones #390

Merged

nylen reviewed Nov 23, 2015
View reviewed changes

aduth added 10 commits November 24, 2015 14:00

Sharing: Implement Redux-based Publicize actions and stores

76bc322

Sharing: Integrate Redux-based Publicize to editor sharing accordion

5f5c917

Framework: Allow store to be initialized on-demand

7d8b3fc

Sharing: Extract connections filtering to separate helper function

e461dc5

Sharing: Move action types from global file to module-specific

50586ea

Framework: Prevent reinitializing redux store per route change

8e28055

Sharing: Map subset of state tree in connect mapper

8bdd3d7

Sharing: Ensure connections exist when new props received

9c39f0e

Resolves the case where if the site were to change while the component is still mounted, a connection fetch would not be triggered if those connections had not already been fetched.

Framework: Add license crediting for Redux packages

1220fa3

Sharing: Allow and guard against empty site

c97533c

Can occur when navigating from a site-specific page to a non-site-specific page, during which time the selected site is unset while the component is still mounted.

aduth force-pushed the update/sharing-redux branch from 57b7d3c to c97533c Compare November 24, 2015 19:01

jkudish added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 24, 2015

aduth added a commit that referenced this pull request Nov 24, 2015

Merge pull request #338 from Automattic/update/sharing-redux

20792e1

Sharing: Implement Redux-based Publicize reducers and actions

aduth merged commit 20792e1 into master Nov 24, 2015

aduth deleted the update/sharing-redux branch November 24, 2015 19:12

scruffian removed the [Status] Ready to Merge label Nov 24, 2015

aduth mentioned this pull request Nov 25, 2015

Sharing: Remove SharingConnectionsData component #736

Merged

jkudish mentioned this pull request Nov 26, 2015

Exporter: Move data into the Redux store #500

Merged

mcsf mentioned this pull request Nov 26, 2015

Themes: Switch data fetching to Redux #840

Merged

2 tasks

jkudish mentioned this pull request Nov 26, 2015

Editor: Update sharing container to use selected site from Redux state #756

Merged

aduth mentioned this pull request Nov 26, 2015

Editor: Sharing: Partially revealing non-shared connections from other users #867

Closed

aduth mentioned this pull request Dec 8, 2015

Notices: Redux everything! #1393

Closed

aduth mentioned this pull request Jan 12, 2016

Sharing: Update sharing Redux state to recommended action type naming #2317

Merged

aduth mentioned this pull request Feb 19, 2016

State: Filter connections by site via selector in place of reducer key #3417

Merged

mtias mentioned this pull request Apr 27, 2016

Framework: Redux single data tree and query components #5046

Closed

lancewillett added State and removed State labels Feb 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sharing: Implement Redux-based Publicize reducers and actions #338

Sharing: Implement Redux-based Publicize reducers and actions #338

aduth commented Nov 20, 2015

jkudish commented Nov 21, 2015

jkudish Nov 21, 2015

aduth Nov 22, 2015

jkudish commented Nov 21, 2015

jkudish commented Nov 21, 2015

jkudish commented Nov 21, 2015

jkudish Nov 21, 2015

aduth commented Nov 22, 2015

nylen Nov 23, 2015

aduth Nov 23, 2015

aduth Nov 23, 2015

jkudish Nov 23, 2015

aduth Nov 23, 2015

nylen commented Nov 23, 2015

aduth commented Nov 23, 2015

jkudish commented Nov 23, 2015

jkudish commented Nov 24, 2015

aduth commented Nov 24, 2015

Sharing: Implement Redux-based Publicize reducers and actions #338

Sharing: Implement Redux-based Publicize reducers and actions #338

Conversation

aduth commented Nov 20, 2015

jkudish commented Nov 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkudish commented Nov 21, 2015

jkudish commented Nov 21, 2015

jkudish commented Nov 21, 2015

Choose a reason for hiding this comment

aduth commented Nov 22, 2015

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

nylen commented Nov 23, 2015

aduth commented Nov 23, 2015

jkudish commented Nov 23, 2015

jkudish commented Nov 24, 2015

aduth commented Nov 24, 2015