Revisions #2: Revisions state update and diff-ing #14241

bperson · 2017-05-18T21:27:47Z

This is the second chunk coming from my WIP PR to introduce post revisions in calypso: #13367. It pushes the normalisation of revisions a bit further and adds support for diff-ing between revisions

changes

It builds upon what was introduced in #12733, adding:

Normalise dates from a format with timezone information in the field names (date_gmt) to a format with timezone information embedded in the datetime strings (simply adding the zone designator for the zero UTC offset: Z). This means we can keep strings in Redux ( instead of moment.js objects) and still use components parsing our strings without any timezone issue.
Add display/editing normalisation updates
Introduce jsdiff as a dependency (no shrinkmap yet, Data Layer: Save response headers in the action meta when redispatching #14157 need to be merged first).

We compute the diff between revisions:

when loading the revisions from the API, saving the changes summary ( words added / words removed ) directly in Redux
when displaying a revision in the diff viewer (coming in the last PR of this serie), it re-computes the complete diff to display all changes, memoized via the getPostRevisionChanges selector.

testing

Besides unit tests, this is currently used nowhere. You can see how it can be used in the WIP PR #13367, for instance in the diff viewer.

bperson · 2017-05-25T13:01:16Z

This has been rebased on the latest add/fetch-users, I still need to get #13828 merged first to provide requestUsers but most of it is standing up on its own.

cc @aduth @nylen or if you know anyone else that might be interested in reviewing (mostly) tests ;)

youknowriad · 2017-06-02T11:26:17Z

client/state/data-layer/wpcom/posts/revisions/index.js

+	forEach( normalizedRevisions, ( revision, index ) => {
+		revision.changes = index === normalizedRevisions.length - 1
+			? { added: 0, removed: 0 }
+			: countDiffWords( diffWords( normalizedRevisions[ index + 1 ].content, revision.content ) );


Curious what's the reasoning behind computing the diff when fetching the revisions? Naively, I'd have let this for a memoized selector, for example, assuming this can be heavy and compute the diff only when needed.

Maybe not needed but what if we want to diff an old revision with the last one for example.

I went this way precisely because it's quite expansive [0]: persisting the summary results along the revisions speeds up the first draw of the revisions list after a reload compared to the equivalent memoized-selector-based solution. That said, it's also partially un-normalizing the revision data in Redux as you get redundant data (the summary can be recomputed from the contents).

Maybe not needed but what if we want to diff an old revision with the last one for example.

The ability to diff between any 2 revisions is dropped for now (internal reference: p4TIVU-6pW-p2#comment-7039). That said, for those features outside the "main" one, it's quite easy to recompute the diff between two revisions with the utils functions introduced here.

As an interesting footnote here: we can most likely go even further and only update the Redux state if we receive deleted or new revisions if we make the assumption that revisions are immutable, AFAICS, there is no way of editing a revision from the API or in wp-admin.

[0] Myers algorithm (used by jsdiff) has a pretty interesting time complexity for revisions like object: it's in O(ND) with N the sum of the number of words in revisions A and B, and D the size of the shortest edit sequence A -> B. If we suppose that most revisions are incremental updates on a post, D should stay small :)

youknowriad · 2017-06-02T11:27:42Z

client/state/data-layer/wpcom/posts/revisions/index.js

 	dispatch( receivePostRevisionsSuccess( siteId, postId ) );
-	dispatch( receivePostRevisions( siteId, postId, map( revisions, normalizeRevision ) ) );
+	dispatch( receivePostRevisions( siteId, postId, normalizedRevisions ) );
+	dispatch( requestUsers( siteId, uniq( map( normalizedRevisions, 'author' ) ) ) );


Do we really want to query the users as soon as we receive the revisions? My first thought is that we should probably use a query component for this when needed. Is there a particular reason for this to be fetched here?

The initial reason for it to be in there was to bundle revisions and users by dispatching receivePostRevisions, receivePostRevisionsSuccess and receiveUsers in the same tick. But I dropped it, from a UX pov, having the author names is nice but not as important as accessing the revisions asap: it's better to have a first render of the revisions while we request more information about the various authors imho.

I've updated this PR to remove this and will probably bundle QueryUsers with its "usage" in the last PR for revisions.

youknowriad · 2017-06-02T11:28:50Z

package.json

@@ -48,6 +48,7 @@
    "creditcards": "2.1.2",
    "cross-env": "4.0.0",
    "debug": "2.2.0",
+    "diff": "1.4.0",


Should we update the lock https://wpcalypso.wordpress.com/devdocs/docs/shrinkwrap.md?term=shrinkw ?

Nevermind, I just saw your note above

Since this is not depending on #13367 anymore, this can be merged on its own, I've updated the shrinkmap :)

… update `date` and `modified` to contain timezone information in their values

… hydratation

bperson · 2017-06-04T21:57:52Z

Updated

removed all usage of requestUsers in favor of using a QueryUsers component alongside QueryPostRevisions in the last PR for revisions
it's rebased on the latest master. It doesn't directly need anything introduced by add/fetch-users anymore, so I updated the shrinkmap, making this mergeable on its own, no issue running with the last shrinkmap locally.

youknowriad

LGTM 👍

It'd be good to have a second opinion.

aduth · 2017-06-05T18:54:24Z

client/lib/text-utils/index.js

+/**
+ * External Dependencies
+ */
+import JsDiff from 'diff';


Can we import { diffWords } from 'diff'; here instead of accessing property below?

we definitely can :) fixed

aduth · 2017-06-05T18:54:50Z

client/lib/text-utils/index.js

@@ -24,4 +30,26 @@ function countWords( content ) {
 	return 0;
 }

-export default { countWords };
+const diffWords = JsDiff.diffWords;


Minor: Destructuring could help avoid some repetition: const { diffWords } = JsDiff;

removed it completely as we're directly importing (and re-exporting in the "default" object diffWords)

aduth · 2017-06-05T18:55:51Z

client/lib/text-utils/index.js

+		diffChanges,
+		( accumulator, change ) => {
+			const count = countWords( change.value );
+			change.added && ( accumulator.added += count );


Minor: I found myself staring at this function for minutes convinced we'd be inaccurately incrementing both added and removed for each change, until I noticed that the first half of each line is treated effectively as a condition. This is one case where I'd advocate for the less magical if ( change.added ) { variation.

I've no strong feelings about either so I updated it to use ifs

aduth · 2017-06-05T18:58:59Z

client/lib/text-utils/index.js

+	);
+}
+
+export default {


Minor and not related to these changes specifically: Exporting only a default of an object makes it difficult to tree shake (dead code elimination) unused functions within a file. We're not yet on Webpack 2 (#10799, #11204) so this isn't entirely relevant yet anyways, but in anticipation it's typically better to export/import functions individually.

TIL :)

I don't really want to touch the imports of countWords if at all possible in this PR ;) Keeping it on my todo once this PR is merged though.

Created a new PR to make sure we don't drop this: #14794

it's based on this one so we should merge this one first.

aduth · 2017-06-05T19:00:24Z

client/state/data-layer/wpcom/posts/revisions/index.js

+		...flow(
+			pick( [ 'date_gmt', 'modified_gmt' ] ),
+			mapValues( val => `${ val }Z` ),
+			mapKeys( key => key.slice( 0, -'_gmt'.length ) )


Minor: Readability: I think it's nice that this isn't a magic number, but at the same time the - is a bit foreign at first glance. Wondering if either -1 * '_gmt'.length or -4 /* "_gmt" */ would be more readable. What do you think?

if we want to truly explain why the -1: -4 /* truncate "_gmt" */? I don't see any way to give the "truncate" meaning to the -1 in code?

aduth · 2017-06-05T19:01:42Z

client/state/posts/revisions/selectors.js

@@ -1,21 +1,74 @@
 /**


All new selectors should ideally be added to the client/state/selectors directory. selectors.js is a legacy pattern.

https://github.com/Automattic/wp-calypso/blob/master/docs/our-approach-to-data.md#selectors

aduth · 2017-06-05T19:03:29Z

client/state/posts/revisions/selectors.js

+
+	return {
+		...revision,
+		...{ author }


This can be simplified as just author.

return { ...revision, author };

…e changes after diff-ing

aduth · 2017-06-06T20:42:06Z

client/state/data-layer/wpcom/posts/revisions/index.js

 		...flow(
 			pick( [ 'title', 'content', 'excerpt' ] ),
 			mapValues( ( val = {} ) => val.rendered )
+		)( revision ),
+		...flow(
+			pick( [ 'date_gmt', 'modified_gmt' ] ),


I'm bewildered by how this is working, specifically the flow/currying applied to pick. In my own Node console it errors as I'd expect it to, because pick isn't supposed to return a function but rather the object subset:

n_ > _.flow( _.pick( 'foo' ) )( { foo: 'bar' } ) TypeError: Expected a function

Can you explain what I'm missing?

first guess would be: _.pick is the non-fp version? which means it returns an empty object {}, and flow doesn't like non-functions as an arg?

well, I'm not answering the question in fact :)

Lodash FP follows a few "defaults" ideas, one of them is to re-order arguments in a "data-last" fashion: _.pick( { foo: 'bar' }, 'foo' ) === fpPick( 'foo' )( { foo: 'bar' } )

Once that's established, what you're missing is probably that the pick you see in the code is the FP version not the "classical" one :)

But pick here is imported from lodash, not lodash/fp.

And to this point, I don't think we'd want to reintroduce lodash/fp, as we'd made a point previously to try to remove it (#11354).

Oh, it is imported from lodash/fp. I totally missed that. Hmm!

Thinking a bit more about it, I haven't found any usage of Lodash FP in calypso (no import, no usage inside a flow AFAICS), it might not be a good idea to start importing it (even more so if people aren't familiar with it since it can cause confusions like this one ;) and the FP docs aren't amazing :( )

well didn't refresh :) Will fix it to use the "classical" one

Yeah, I actually quite like how this looks here. It's nice to compose the operations this way. But I'd agree we might not want to set this precedent for risk of confusing developers with the presence of both variants (i.e. being consistent on using one vs. the other).

aduth

Looks good 👍

bperson added the [Status] In Progress label May 18, 2017

matticbot added OSS Citizen [Size] XL Probably needs to be broken down into multiple smaller issues labels May 18, 2017

bperson changed the title ~~Revisions #2: State Revisions update and diff-ing~~ Revisions #2: Revisions state update and diff-ing May 19, 2017

bperson requested review from aduth and nylen and removed request for aduth May 19, 2017 13:37

bperson 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 May 19, 2017

bperson force-pushed the add/fetch-users branch from abb18c8 to 31536b4 Compare May 25, 2017 12:45

bperson mentioned this pull request May 25, 2017

Revisions #1: State > Users: Add USERS_REQUEST action w/ data-layer support #13828

Merged

bperson force-pushed the update/state-revisions branch from bcda7f6 to defb645 Compare May 25, 2017 12:58

youknowriad reviewed Jun 2, 2017

View reviewed changes

bperson added 2 commits June 4, 2017 23:31

State > Data Layer > Revisions: Remove date_gmt and modified_gmt,…

e369792

… update `date` and `modified` to contain timezone information in their values

State > Revisions: Normalization updates for display/editing + Author…

5c995fe

… hydratation

bperson force-pushed the update/state-revisions branch from defb645 to 1cf76fe Compare June 4, 2017 21:35

bperson changed the base branch from add/fetch-users to master June 4, 2017 21:35

State > Revisions: Add support for word-diff between revisions

a9cc31b

bperson force-pushed the update/state-revisions branch from 1cf76fe to a9cc31b Compare June 4, 2017 21:56

youknowriad approved these changes Jun 5, 2017

View reviewed changes

aduth reviewed Jun 5, 2017

View reviewed changes

bperson added 3 commits June 5, 2017 22:01

Lib > Text Utils: Fix import of jsDiff's diffWords

3b3794e

Lib > Text Utils: Fix obfuscated reduction function used to accumulat…

137045c

…e changes after diff-ing

State (> Revision): Fix using legacy pattern for selectors

0ae6f64

bperson mentioned this pull request Jun 6, 2017

Text Utils: Fix default exporting an object, preventing tree shaking #14794

Merged

aduth reviewed Jun 6, 2017

View reviewed changes

State > Revisions: Use "classical" lodash instead of fp one

43005e9

aduth approved these changes Jun 7, 2017

View reviewed changes

bperson merged commit 9fa4f44 into master Jun 8, 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 Jun 8, 2017

bperson deleted the update/state-revisions branch June 8, 2017 10:29

bperson mentioned this pull request Jun 8, 2017

Revisions #6: Fetch authors #14930

Merged

deBhal mentioned this pull request Nov 14, 2017

Calypso/Editor: Freeze almost immediately after loading a page #19745

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revisions #2: Revisions state update and diff-ing #14241

Revisions #2: Revisions state update and diff-ing #14241

bperson commented May 18, 2017 •

edited

Loading

bperson commented May 25, 2017

youknowriad Jun 2, 2017

bperson Jun 4, 2017 •

edited by designsimply

Loading

youknowriad Jun 2, 2017

bperson Jun 4, 2017

youknowriad Jun 2, 2017

youknowriad Jun 2, 2017

bperson Jun 4, 2017

bperson commented Jun 4, 2017

youknowriad left a comment

aduth Jun 5, 2017

bperson Jun 5, 2017

aduth Jun 5, 2017

bperson Jun 5, 2017

aduth Jun 5, 2017

bperson Jun 5, 2017

aduth Jun 5, 2017

bperson Jun 5, 2017

bperson Jun 6, 2017

aduth Jun 5, 2017

bperson Jun 5, 2017

aduth Jun 5, 2017

bperson Jun 5, 2017

aduth Jun 5, 2017

bperson Jun 5, 2017

aduth Jun 6, 2017

bperson Jun 6, 2017

bperson Jun 6, 2017 •

edited

Loading

aduth Jun 6, 2017

aduth Jun 6, 2017

bperson Jun 6, 2017 •

edited

Loading

bperson Jun 6, 2017

aduth Jun 6, 2017

aduth left a comment

Revisions #2: Revisions state update and diff-ing #14241

Revisions #2: Revisions state update and diff-ing #14241

Conversation

bperson commented May 18, 2017 • edited Loading

changes

testing

bperson commented May 25, 2017

Choose a reason for hiding this comment

bperson Jun 4, 2017 • edited by designsimply 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

bperson commented Jun 4, 2017

Updated

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

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

bperson Jun 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

bperson Jun 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

aduth left a comment

Choose a reason for hiding this comment

bperson commented May 18, 2017 •

edited

Loading

bperson Jun 4, 2017 •

edited by designsimply

Loading

bperson Jun 6, 2017 •

edited

Loading

bperson Jun 6, 2017 •

edited

Loading