Add QueryMedia component connected to a Redux store. #10392

ellatrix · 2017-01-03T22:48:19Z

~~N.b.: This includes #10386.~~ Merged!

Add QueryMedia component.
Add Redux state using MediaQueryManager.
Merge existing (simple) media store.

Split from #10352.
This is a task from #5046.

ellatrix · 2017-01-03T23:15:52Z

I would need some feedback on "Merge existing (simple) media store", because I'm not sure if this should be kept or not. I see that the posts store has an items store as well, even though there is a query store. The items store was added in #10016. Cc @aduth.

aduth · 2017-01-04T01:02:16Z

In the case of posts, since we'd opted to enable keying and lookup by global ID, items exists as simply a mapping of global IDs to [ siteId, postId ] tuples. Since a media item does not have a global ID, we should be able to get away with only an items state subtree, though in order to perform rich query management like in posts, we may need to implement it as an instance of MediaQueryManager. See posts reducer as an example, though it's complex enough that I wonder if we can abstract any of it.

(Caveat to my comments being this is made in passing, I've not yet taken a close look at the changes here)

aduth · 2017-01-04T01:05:36Z

client/state/data-layer/wpcom/sites/media/index.js

+ * @return {XMLHttpRequest}        XMLHttpRequest
+ */
+export function requestMedia( { dispatch }, { siteId, query } ) {
+	dispatch( requestingMedia( siteId, query ) );


Do you have on thoughts on whether the isRequestingMedia check currently in the query component would make more sense in the API middleware handler?

Yes, that sounds like a good idea.

aduth · 2017-01-04T01:07:15Z

client/state/data-layer/wpcom/sites/media/index.js

+export function requestMedia( { dispatch }, { siteId, query } ) {
+	dispatch( requestingMedia( siteId, query ) );
+
+	return wpcom.site( siteId ).mediaList( query, function( error, data ) {


At the very least I might suggest the more concise ES6 arrow function syntax for the anonymous function, though further I wonder if we should treat the return value as a promise. This would be difficult to test as-is, for example, since nock stubbing doesn't complete predictably in the current or next tick.

Yes, planning to do this. :)

aduth · 2017-01-04T01:08:27Z

client/state/media/reducer.js

@@ -36,6 +40,52 @@ export const items = createReducer( {}, {
 	}
 } );

+export const queries = ( () => {
+	function applyToManager( state, siteId, method, createDefault, ...args ) {


Maybe I should have glanced at the changes before writing my initial comment 😆 But in this case I think we don't need both items and queries. A QueryManager instance allows us to retrieve a single item, all items, or items by queries, which should satisfy all of our selector requirements.

aduth · 2017-01-04T01:11:19Z

client/state/media/reducer.js

+	} );
+} )();
+
+export const queryRequests = keyedReducer( 'siteId', createReducer( [], {


It's really not obvious, and I encountered it myself the hard way, but composing a keyedReducer createReducer will persist its state even though one would expect createReducer to only persist if a schema is provided.

Related: #10016 (comment)

Ah, thanks for the tip. I was trying to figure out why this happened. :)

aduth · 2017-01-04T01:12:49Z

client/state/selectors/is-requesting-media.js

+	const queryRequests = state.media.queryRequests[ siteId ];
+	const stringifiedQuery = MediaQueryManager.QueryKey.stringify( query );
+
+	if ( ! queryRequests ) {


Since we've not yet used stringifiedQuery at this point where an early return could occur, and QueryKey.stringify is not exactly trivial, it might be better to declare after the condition.

aduth · 2017-01-04T01:13:24Z

client/state/selectors/is-requesting-media.js

+		return false;
+	}
+
+	return queryRequests.indexOf( stringifiedQuery ) !== -1;


Personal preference, but I find _.includes to be a more human-readable equivalent, which also happens to automatically account for a falsey queryRequests.

Hm, according to the documentation, it does not accept a boolean collection argument.

Hm, according to the documentation, it does not accept a boolean collection argument.

Sorry, I should have been more clear. Since the issue is we're trying to avoid calling indexOf when queryRequests is potentially null, I meant to point out that _.includes will automatically account for this and return false.

FWIW, though not documented, I did test and confirm that it behaves as expected if passed a boolean:

n_ > _.includes( false, 'foo' ); false

aduth · 2017-01-04T01:14:07Z

client/components/data/query-media/index.jsx

+}
+
+QueryMedia.propTypes = {
+	siteId: PropTypes.number,


Should siteId be required? What happens if we pass a falsey value?

Yeah, I was thinking about this when adjusting the documentation, but then left it the same as PostsQuery. Should probably be updated there too.

aduth · 2017-01-04T01:17:55Z

client/state/media/actions.js

 * @return {Object}                Action object
 */
-export function receiveMedia( siteId, media ) {
+export function receiveMedia( siteId, media, found, query ) {


Failing test on this action creator, likely not expecting the additional properties.

I know, wanted to do this while merging the the reducers and tests. Probably the worst idea to let tests fail after a commit. 😶

aduth · 2017-01-04T01:18:55Z

client/state/media/actions.js

 	return {
 		type: MEDIA_RECEIVE,
+		siteId,


It's a bit arbitrary and conventions vary, but I tend to find it more "pleasing" to read when the shorthand properties are grouped together separate from properties with values defined (either top or bottom of object). What do you think?

I was torn between that and having the same order as the arguments (and other action creators).

my arbitrary preference is for alphabetical arrangement 😛

ellatrix · 2017-01-04T15:02:39Z

client/state/data-layer/wpcom/sites/media/index.js

+
+		log( 'Request media for site %d using query %o', siteId, query );
+
+		return wpcom.site( siteId ).mediaList( query, function( error, data ) {


To do: return a promise and dispatch failures.

ellatrix · 2017-01-04T15:07:35Z

client/components/data/query-media/README.md

+
+<table>
+	<tr><th>Type</th><td>Number</td></tr>
+	<tr><th>Required</th><td>No</td></tr>


To do: this is required now.

ellatrix · 2017-01-06T09:04:34Z

I see that there is a schema in state/posts, so it should probably be added here too. It looks like it would belong better with the query manager though, as that's creating the data structure.

dmsnell

Thanks for using the newer API middleware @iseulde! This is all looking very nice. Please let me know if I can be a help to you on any of my feedback.

dmsnell · 2017-01-06T17:02:32Z

client/components/data/query-media/index.jsx

+QueryMedia.defaultProps = {
+	request: () => {}
+};
+


as a note on style, we have been putting these inside the React classes as static properties

class extends Component { static propTypes = { blarg: 'woohoo', }; static defaultProps = { translate: noop, }; }

Cool, will changed it. I wasn't sure what's preferred, and I only saw this style so far in the codebase.

dmsnell · 2017-01-06T17:04:41Z

client/components/data/query-media/index.jsx

+		return bindActionCreators( {
+			request: requestMedia
+		}, dispatch );
+	}


fun fact! if we have a simple list of functions to bind to action names then we can simply provide an object whose keys are the action name and whose values are the actual functions to call

const mapDispatchToProps = ( { request: requestMedia, } ); export default connect( null, mapDispatchToProps )( QueryMedia );

Thanks for the tip. I also put mapDispatchToProps at the top of the file as it makes it easier to understand where the props of the component are coming from. I can move this back to the bottom if that's preferred.

dmsnell · 2017-01-06T17:07:47Z

client/state/data-layer/wpcom/sites/media/index.js

+ *
+ * @param  {Object}  store  Redux store
+ * @param  {Object}  action Action object
+ * @return {Promise}        Promise


While I appreciate the comment Promise is somewhat redundant 😉 Maybe something a bit more descriptive could help here, or we could just leave it off.

/** * @return {Promise} wpcom.js request response */

Leaving it off will give an eslint warning, which is why I added it here. Similarly "Action object" for {Object} action is somewhat redundant too. :)

this is one thing that bothers me about linters - they tend to push us towards caring about the wrong things. the linter says, "nothing is there" so we put something there, but the intention of the linter was to motivate us to provide a helpful description.

my personal habits tend to be a little more aggressive against linters in this regard, where I might ignore its noise or try and fix the linting strategy.

in this case, even adding a single word to it, like "Action object" helps a lot, even more so potentially something like, "Redux action"

dmsnell · 2017-01-06T17:09:31Z

client/state/data-layer/wpcom/sites/media/index.js

+			dispatch( receiveMedia( siteId, media, found, query ) );
+		} ).catch( () => {
+			dispatch( failMediaRequest( siteId, query ) );
+		} );


Although it's a personal style request and I don't think we have this in our guidelines, would you be interested in breaking this into multiple lines? at a minimum so that .then and .catch start on their own lines? (You can feel free to say "no" to this)

return wpcom .site( siteId ) .mediaList( query ) .then( ( { media, found } ) => dispatch( receiveMedia( siteId, media, found, query ) ) ) .catch( () => dispatch( failMediaRequest( siteId, query ) ) );

dmsnell · 2017-01-06T17:09:52Z

client/state/media/actions.js

+	MEDIA_RECEIVE,
+	MEDIA_REQUEST,
+	MEDIA_REQUEST_FAILURE,
+	MEDIA_REQUESTING } from 'state/action-types';


how about a trailing comma and a newline before the }?

Sure, but what's the guideline for this? https://wpcalypso.wordpress.com/devdocs/docs/coding-guidelines/javascript.md#objects

I personally like trailing command as well. For the same reasons I like separate var statements instead of comma separating them.

var something var somethingElse

But I'll follow whatever is preferred.

Similarly I would prefer this:

<MediaLibrary prop={ prop } prop={ prop } />

Even if it looks ugly. :) But haven't seen that so far.

dmsnell · 2017-01-06T17:10:32Z

client/state/media/actions.js

 	return {
 		type: MEDIA_RECEIVE,
+		siteId,


my arbitrary preference is for alphabetical arrangement 😛

dmsnell · 2017-01-06T17:11:23Z

client/state/media/reducer.js

+			return applyToManager( state, siteId, 'removeItems', true, mediaIds );
+		}
+	} );
+} )();


This IIFE is a bit curious to me here. Can you clarify what it's being used for?

Again, this is from state/posts.

This IIFE is a bit curious to me here. Can you clarify what it's being used for?

A questionable technique of scoping and colocating related pieces.

dmsnell · 2017-01-06T17:24:37Z

client/state/media/reducer.js

+			...state,
+			[ siteId ]: nextManager
+		};
+	}


@aduth would you agree that this might be a good place to look into using the keyedReducer()?

wp-calypso/client/state/utils.js

Line 77 in 024095d

export const keyedReducer = ( keyName, reducer ) => {

I'm a bit distracted by some of the code, but @iseulde the idea here is that we can identify shape of the state for an item in a collection and then compose that item reducer with this utility to keep things well-separated and keep the shape of the underlying data transparent (vs. hidden behind initialState = {})

I copied applyToManager from state/posts :) Not sure what's best here. Looks like it could be abstracted. See #10392 (comment).

no worries @iseulde - I'm not as familiar with the code history.

please ignore this then. no need to conflate behavioral changes with code moves.

dmsnell · 2017-01-06T17:27:51Z

client/state/selectors/get-media.js

+		return null;
+	}
+
+	return queries.getItems( query );


What is this query object that we are storing? It appears like it contains a function, but if indeed we are storing functions inside of the Redux state tree we should be very skeptical about it. The state tree is best and ideal when only primitive data and compositions thereof are inside of it. Data is translatable, but functions aren't.

Isn't this what the query managers are for?

Isn't this what the query managers are for?

What is "this"

What is "are for?"

It may be that the move into Redux demands some changes to the way we think about these things. In fact, if the query manager is working on its own data, a trivial change might be writing a fully-independent getItems()-type function which accepts the actual object being operated on as an input argument.

Keeping functions out of the state is pretty high up on the list of priorities for state design.

What is "this"
What is "are for?"

Sorry, let me put it differently: Are the query managers not designed to manage Redux state? Sorry if they aren't, I saw it being used for posts and adopted it here. I'll think about how to do it differently then.

Is there any broader discussion (ticket) for removing the use of query managers?

When it comes down to it though, when we store Immutable objects in state, we're storing an instance of its class, so I don't see much of a difference between something like state.values() (of an instance of Immutable.Map) vs. state.getItems() (of an instance of MediaQueryManager). You do raise a good point that Immutable objects can be converted Immutable <-> JS <-> String whereas QueryManager instances can only be converted QueryManager <-> String, though in practice we don't (and probably shouldn't) make use of the JavaScript form of an Immutable instance, except perhaps when initializing from an object.

Would it be much of a difficult task to turn stateObject.getItems( query ) into getItems( stateObject, query )?

I'm sure it could be done, but maintaining a valid state object shape and passing the required arguments for each argument is not nearly as easy to use as it is in its class form:

const nextManager = receive( { items: {}, queries: {} }, { itemKey: 'ID' }, posts, query ); // vs. const nextManager = new PostQueryManager().receive( posts, query );

Also, in case it lends anything to the argument, operations on the QueryManager class are immutable, i.e. one can rely upon strict equality between two instances to determine whether the contents are in-fact equal.

https://github.com/Automattic/wp-calypso/blob/31ff257/client/lib/query-manager/test/index.js#L199-L283

okay @aduth. I sense that there is far more behind the QueryManager than I understand, though I don't think that understanding would change my opinion on whether or not it should be here in state. it's not actually state. it's something that operates on state and isn't directly linked to representing the data to the user.

that being said, I can let this slide in my mind as an exception since it's all existing code. it's an exception to an exception though because we're adding it fresh into state right now instead of refactoring existing stuff in state.

it's not altogether different than when using Immutable.js and that's the source of my love/hate relationship with it. it's not actually JavaScript data and it's awkward no matter how you slice it when interoperating with other normal JavaScript objects. they have chosen performance over native representation and I get that.

I will think about this a bit over the weekend and see if I can come up with an alternative. I guess the query managers have too much logic that should go to reducers and selectors.

Are we blocked here by whether or not we should use a query manager for media items?

For more history I believe the original PR was #5135

See also #5289

youknowriad

Curious about the status of this PR. Is it blocked? How can we move this forward?

I'm asking because I've found myself needing a media redux subtree in my current WIP and I didn't want to duplicate the efforts.

youknowriad · 2017-01-19T12:55:22Z

client/components/data/query-media/index.jsx

+	}
+}
+
+export default connect( null, mapDispatchToProps )( QueryMedia );


could we provide a way to query medias by Id, the same way we do in QueryPosts by providing a postId prop ?

Granted this could be done in a separate PR.

youknowriad · 2017-01-19T12:58:09Z

client/state/data-layer/wpcom/sites/media/index.js

+ * @return {Promise}        Promise
+ */
+export function requestMedia( { dispatch, getState }, { siteId, query } ) {
+	if ( ! isRequestingMedia( getState(), siteId, query ) ) {


Personal preference, but I would inverse the condition if ( isRequesting ) { return; }

youknowriad

I rebase this, and it should be ok to merge now 👍

See 786a726.

… action creators if it's a plain object. Additionally put it on the top of the file for better readability.

aduth · 2017-01-31T14:43:43Z

client/state/media/actions.js

+ * @param  {Object} query  Query object
+ * @return {Object}        Action object
+ */
+export function requestingMedia( siteId, query ) {


Why are there separate action creators requestMedia and requestingMedia? I don't see any meaningful difference in how they're used.

Actually there is a difference: requestMedia is responsible of triggering the middleware handler. While requestingMedia tells that we are effectively starting the Network Request. Because the middleware could decide to avoid triggering a request if we're already requesting the same request.

I guess we'll see more of those while moving to the data middleware

aduth · 2017-01-31T14:55:55Z

Aside from note above, this appears to look and work well to me. 👍

youknowriad · 2017-02-01T11:55:27Z

I'm merging this and I'll follow up with a QueryMedia by ID PR

ellatrix added Components [Feature] Media The media screen in Calypso, general media management, or integration with third party media. State [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Task labels Jan 3, 2017

matticbot added the OSS Citizen label Jan 3, 2017

aduth reviewed Jan 4, 2017

View reviewed changes

ellatrix force-pushed the add/query-media-component branch from 082c2b3 to 7dff526 Compare January 4, 2017 10:14

ellatrix commented Jan 4, 2017

View reviewed changes

ellatrix requested review from aduth and dmsnell January 6, 2017 09:04

ellatrix mentioned this pull request Jan 6, 2017

Reduxify Media Library #10352

Closed

4 tasks

ellatrix force-pushed the add/query-media-component branch from acc3217 to baab6a1 Compare January 6, 2017 13:51

ellatrix mentioned this pull request Jan 6, 2017

Add Redux equivalent of MediaLibrarySelectedStore #10458

Closed

ellatrix requested a review from gwwar January 6, 2017 15:47

dmsnell reviewed Jan 6, 2017

View reviewed changes

youknowriad reviewed Jan 19, 2017

View reviewed changes

youknowriad force-pushed the add/query-media-component branch from efa3d6b to e27ade2 Compare January 30, 2017 08:46

youknowriad approved these changes Jan 30, 2017

View reviewed changes

youknowriad 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 Jan 30, 2017

ellatrix added 5 commits January 31, 2017 15:00

Add QueryMedia component connected to a Redux store.

f8285a2

Adjust failing reducer test.

1b987a4

Add and adjust action creator tests.

135358d

Require siteId for QueryMedia.

1b2457e

Move requesting check from query component to middleware.

7ec6226

ellatrix and others added 13 commits January 31, 2017 15:00

Move stringifiedQuery calculation after early return.

a8f787c

Use Lodash's includes for better readability.

84a2123

Add tests for isRequestingMedia selector.

ebe3301

Add tests for getMedia selector.

b3bf488

Add tests for queryRequests reducer.

d6b60a3

Do not persist queryRequests state.

0de4ffc

Merge items and queries reducers.

12ada82

Update docs after requiring siteId for QueryMedia.

dae25ed

See 786a726.

Take request failures into account.

6c6f984

Declare propTypes and defaultProps as static properties.

8876fa1

mapDispatchToProps will automatically be assumed to be an object with…

7728fdf

… action creators if it's a plain object. Additionally put it on the top of the file for better readability.

Changes per review

1c77cea

Fix isTransientMedia unit test

ac59215

youknowriad force-pushed the add/query-media-component branch from d370dd1 to ac59215 Compare January 31, 2017 14:05

aduth reviewed Jan 31, 2017

View reviewed changes

aduth mentioned this pull request Jan 31, 2017

Site Settings: Show error notice when icon fails to upload #11043

Merged

youknowriad merged commit db6188c into master Feb 1, 2017

youknowriad deleted the add/query-media-component branch February 1, 2017 11:56

matticbot removed the [Status] Ready to Merge label Feb 1, 2017


		log( 'Request media for site %d using query %o', siteId, query );

		return wpcom.site( siteId ).mediaList( query, function( error, data ) {

Add QueryMedia component connected to a Redux store. #10392

Add QueryMedia component connected to a Redux store. #10392

Conversation

ellatrix commented Jan 3, 2017 • edited Loading

ellatrix commented Jan 3, 2017

aduth commented Jan 4, 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

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

ellatrix Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Jan 6, 2017

dmsnell left a 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

ellatrix Jan 7, 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

ellatrix Jan 6, 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

gwwar Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

youknowriad left a 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

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Jan 31, 2017

youknowriad commented Feb 1, 2017

ellatrix commented Jan 3, 2017 •

edited

Loading

ellatrix Jan 4, 2017 •

edited

Loading

ellatrix Jan 7, 2017 •

edited

Loading

ellatrix Jan 6, 2017 •

edited

Loading

gwwar Jan 11, 2017 •

edited

Loading