Framework: Reduxify embeds #38608

tyxla · 2019-12-30T09:03:24Z

It's been a while since we started migrating all the legacy EventEmitter and flux stores to use redux. Most of them have been migrated, but there are still some remaining, mostly big and thorny ones.

This PR creates the embeds Redux infrastructure, and ports the existing embed flux stores usage to Redux. Finally, it removes those legacy flux stores.

This removes the last usages of flux/utils and of flux's ReduceStore. A thorough list of changes can be found in the list below.

I'm happy to split it into multiple PRs, but I didn't because:

The Redux infrastructure is pretty straightforward and simple
A big portion of the added code is tests.
Component reduxification goes well together rather than being split into multiple PRs.
Commits are atomic and are supposed to provide background to the changes.

Note that I didn't create a query component for the list of embed patterns, simply because it wasn't needed.

Changes proposed in this Pull Request

Introduce Redux infrastructure for handling embeds:
- Action creators
- Data layer
- Reducer
- Utils (used for normalization of embed regex patterns)
- Selectors
- Tests for utils, reducer and selectors - not necessary for data layer and action creators as they would be simple dupes of the code that they test.
- Query component for the embed rendering (QueryEmbed)
Reduxify the EmbedView tinymce plugin view component
Reduxify the EmbedViewManager component
Remove legacy flux embed stores.

Testing instructions

Checkout this branch.
Go to the post edit screen for a WP.com site.
Make sure you're using the Calypso post editor instead of Gutenberg.
Try inserting an embeddable URL in your post.
Verify it gets converted to a proper embed in visual mode.
Compare the embeds behavior with what's in production, it should work the same way.
Make sure to test with a clean Redux store.
Verify all tests pass.
If you'd like to test the Redux infrastructure:
- Open your browser console.
- Type dispatch( { type: 'EMBEDS_REQUEST', siteId: 1234 } ) where 1234 is the ID of one of your sites.
- Verify there's a network request to fetch the supported embed regex patterns.
- Type state.embeds.siteItems in your browser console.
- Verify you can see the embed patterns keyed by site ID in the state.
- Type dispatch( { type: 'EMBED_REQUEST', siteId: 1234, url: 'https://www.facebook.com/20531316728/posts/10154009990506729/' } ) where 1234 is the ID of one of your sites.
- Verify there's a network request to fetch the embed render data.
- Type state.embeds.urlItems in your browser console.
- Verify you can see the embed render data in the state.

Fixes #7924

matticbot · 2019-12-30T09:03:27Z

Test live: https://calypso.live/?branch=reduxify/embeds

matticbot · 2019-12-30T09:16:15Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~166767 bytes added 📈 [gzipped])

name                 parsed_size            gzip_size
entry-main             +751609 B  (+84.1%)  +166512 B  (+70.5%)
entry-jetpack-cloud       +795 B   (+0.1%)     +255 B   (+0.1%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~2957 bytes removed 📉 [gzipped])

name         parsed_size           gzip_size
post-editor     -13416 B  (-0.7%)    -2957 B  (-0.5%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

delawski

@tyxla Thank you working on this!

I've tested both visually and in console and embeds seem to work as expected.

I've also left some comments regarding the tests. Please let me know what do you think about the suggestions.

While the Flux to Redux migration details make sense to me, I'm not quite familiar with Flux so it's hard for me to do a thorough review of this piece. In order to get some more orientation, I've read the docs about our approach to data (nb: I've updated some links there - please review this little PR when you get a chance: #38623), but I think that some experience with the technology is needed here first of all.

The PR looks good to me and I'm approving it now with an expectation that you'll have a second look into the test issues I mentioned in the code.

client/state/embeds/test/reducer.js

client/state/selectors/test/get-embed.js

tyxla · 2020-01-02T14:34:13Z

This could use another review, and I'd appreciate anyone from @Automattic/team-calypso taking a look just in case.

jsnajdr · 2020-01-06T09:15:23Z

client/components/tinymce/plugins/wpcom-view/views/embed/view.jsx

@@ -99,7 +93,7 @@ class EmbedView extends Component {
 	}

 	renderFrame() {
-		if ( ! this.state.wrapper ) {
+		if ( ! this.state?.wrapper || ! this.props.embed ) {


It's better to set the initial state to { wrapper: null } and avoid the optional chaining with state?.wrapper.

Good call, done in 10736af748c98669da281919246395762be1fec1.

jsnajdr · 2020-01-06T09:32:45Z

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

+ * @param   {object} response API response
+ * @returns {Array}           Array of embeds.
+ */
+const fromApi = response => response.embeds || [];


Personally, I would avoid creating this fromApi handler, and would move the logic to the EMBEDS_RECEIVE reducer and the normalizeEmbeds function.

I see why you suggest this, but I think I prefer it as-is. The idea here is that we're separating the API endpoint specifics (the fact that the endpoint puts it into a .embeds field) from the actual normalization logic that expects just an array of embed patterns. I like keeping those two separate, and I think that fromApi helps us isolate those concerns elegantly.

jsnajdr · 2020-01-06T09:37:30Z

client/state/embeds/actions.js

+ * Internal dependencies
+ */
+import { EMBED_RECEIVE, EMBED_REQUEST, EMBEDS_RECEIVE, EMBEDS_REQUEST } from 'state/action-types';
+import 'state/data-layer/wpcom/sites/embeds';


After the data layer migration, we can also remove this wpcom.undocumented function that's no longer used:

wp-calypso/client/lib/wpcom-undocumented/lib/site.js

Line 106 in 292d0fc

UndocumentedSite.prototype.embeds = function( attributes, callback ) {

Oh, it seems this one is still used in EmbedDialog. I'd be happy to remove that in a subsequent PR, as this one has grown quite a bit. Does that sound good to you?

Cool, I didn't notice the other usage. Let's not snowball this PR by trying to remove it 👍

I've decided not to do this for now. The way EmbedDialog is architectured right now relies too much on the internals of how wpcom performs requests. To decouple from that and rely on the data layer + traditional redux query component and selector, we'll have to refactor the entire component, which I'd rather strive away from at this time.

jsnajdr · 2020-01-06T13:10:20Z

client/components/tinymce/plugins/wpcom-view/views/embed/index.js


-		this.sitesListener = this.updateSite.bind( this );
+EmbedViewManager.match = content => {


This new implementation of match has two flaws:

First, it dispatches requestEmbeds on each call, even if the embed list is already loaded. The original implementation didn't do that. That means that the /embeds REST request is sent very frequently as the user edits the content.

Second, if on the first call to match the embeds list is not yet loaded, it doesn't find any of the embeds. And when the list load is finished, we need to somehow trigger the match again. The original implementation achieved that by being an EventEmitter instance and becoming part of the emitters list. The new implementation doesn't subscribe to changes in the Redux store at all.

Thanks, really good catch.

I was actually able to repro the issue you're talking about (the second one) by delaying the initial embeds request to take longer. Unfortunately, it seems like the TinyMCE plugins rely on the emitters quite a bit. This is something we can't really get away from in this PR. It's likely a worthy effort, but not for this PR.

So, I've reverted the EmbedViewManager to an EventEmitter, but changed it so it now subscribes to changes of interest in the Redux store. That way, match will be called again whenever the embed patterns are loaded, and the embeds will be properly rendered.

Furthermore, I've modified it so we'll only request site embed patterns once.

Let me know what you think.

jsnajdr · 2020-01-06T13:16:28Z

client/components/tinymce/plugins/wpcom-view/views/embed/index.js

-			actions.fetch( this.siteId );
-		}
-	}
+		reduxDispatch( requestEmbed( siteId, url ) );


The request to render the embed should be a part of the EmbedView component, using the classical query+selector pattern:

function EmbedView() { return ( <div> <QueryEmbed siteId={ ... } url={ ... } /> { renderFrame() } </div> ); } connect( state => ( { embed: getEmbed( state, siteId, url ) } ) );

The match function only needs the list of the site's embed regexps, and is not concerned with embed rendering at all.

Good call, moved the actual embed render request to be handled by a query component that lives in EmbedView.

jsnajdr · 2020-01-06T13:20:10Z

client/components/tinymce/plugins/wpcom-view/views/embed/view.jsx

@@ -81,7 +73,7 @@ class EmbedView extends Component {
 	}

 	setHtml() {
-		if ( ! this.state.body || ! this.refs.iframe ) {
+		if ( ! this.props.embed?.body || ! this.refs.iframe ) {


It shouldn't be hard to convert the refs from strings to modern React.createRef.

Sure 👍 I'd prefer doing that in a follow-up PR though, I hope you don't mind? This one is too big anyway, so I try hard to refrain from any non-critical changes 😉

Addressing this in #39316

jsnajdr · 2020-01-06T13:21:53Z

client/components/tinymce/plugins/wpcom-view/views/embed/index.js

 import { getSelectedSiteId } from 'state/ui/selectors';
-import { SELECTED_SITE_SUBSCRIBE, SELECTED_SITE_UNSUBSCRIBE } from 'state/action-types';


After this PR, the SELECTED_SITE_SUBSCRIBE actions can be removes, as their only usage is gone. You'll still need to subscribe to the Redux store, but that shouldn't be done with store.subscribe(). Layers of indirection with actions and middleware are not needed.

Like the other cleanups, I'm happy to address it in another PR to keep this one as small as possible. It's far from being small already 😉

Addressed in #39285.

tyxla · 2020-02-03T14:45:20Z

@jsnajdr thanks for the thorough review, nice catches.

I've fixed the critical problems you've outlined, while suggesting several follow-up PRs. If that sounds good, I'll follow-up with them in PRs once this one gets merged. They are:

remove .embeds() from EmbedDialog and delete .embeds() from wpcom.site - Framework: Reduxify embeds #38608 (comment)
remove SELECTED_SITE_SUBSCRIBE and SELECTED_SITE_UNSUBSCRIBE - https://github.com/Automattic/wp-calypso/pull/38608/files#r363290099
remove string refs - https://github.com/Automattic/wp-calypso/pull/38608/files#r363289478

This is ready for another review 🙌

jsnajdr

Nice! I'm posting a few suggestions on how to improve the code without changing behavior.

jsnajdr · 2020-02-04T16:05:05Z

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

+	render() {
+		return null;
+	}
+}


To write query components, hooks and the useDispatch hook is an awesome tool:

function QueryEmbeds( { siteId, url } ) { const dispatch = useDispatch(); useEffect( () => { dispatch( requestEmbed( siteId, url ) ); }, [ dispatch, siteId, url ] ); return null; }

And that's the whole component!

I like that, thanks - much simpler 👍

Implemented in d264692

jsnajdr · 2020-02-04T16:41:19Z

client/components/tinymce/plugins/wpcom-view/views/embed/index.js

-			this.updateSite();
-		}
+	getCurrentSiteEmbeds( state ) {
+		return getSiteEmbeds( state, getSelectedSiteId( state ) );


getCurrentSiteEmbeds doesn't need to be a method. It doesn't access this. It can be a standalone function -- a Redux selector like any other.

Makes sense 👍 Created the new selector in 3892ac2 and updated EmbedView to use it in 0417db9

jsnajdr · 2020-02-04T16:44:42Z

client/components/tinymce/plugins/wpcom-view/views/embed/index.js

 	}

-	removeListener( event, listener ) {
-		super.removeListener( event, listener );
+	createListener( store, selector, callback ) {


createListener also doesn't need to be a method. It would be also nice if I could pass an array of selectors:

createListener( store, [ getSelectedSiteId, getCurrentSiteEmbeds ], callback );

That's somewhat similar to how hooks handle dependency arrays.

I think I'd prefer the explicitness of the current way it's written. I hope you wouldn't mind if I keep it as-is? 😉

OK, but I still think it doesn't need to be a method 🙂

…store listeners

tyxla · 2020-02-05T12:27:46Z

Feedback addressed! I think this is ready to go @jsnajdr - wanna give it a 👍 before I 🚢 ? Thank you!

jsnajdr

Still looks good, thanks for the great work here 👍

tyxla added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Task State [Tests] Includes Tests Maintenance labels Dec 30, 2019

tyxla requested a review from a team December 30, 2019 09:03

tyxla self-assigned this Dec 30, 2019

tyxla mentioned this pull request Dec 30, 2019

Reduxify: Migrate lib/embeds #7924

Closed

4 tasks

delawski approved these changes Dec 31, 2019

View reviewed changes

client/state/embeds/test/reducer.js Outdated Show resolved Hide resolved

client/state/selectors/test/get-embed.js Outdated Show resolved Hide resolved

delawski added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 31, 2019

tyxla added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Jan 2, 2020

tyxla requested a review from delawski January 2, 2020 14:34

jsnajdr reviewed Jan 6, 2020

View reviewed changes

tyxla added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 15, 2020

tyxla force-pushed the reduxify/embeds branch from 98f1880 to 1cccda7 Compare February 3, 2020 14:26

tyxla added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Feb 3, 2020

tyxla requested a review from jsnajdr February 3, 2020 14:45

jsnajdr approved these changes Feb 4, 2020

View reviewed changes

tyxla added 17 commits February 5, 2020 13:48

State: Introduce getSiteEmbeds selector

617f72e

State: Add tests for getSiteEmbeds selector

175056f

State: Introduce getEmbed selector

61d7d33

State: Add tests for getEmbed selector

6be914c

Components: Reduxify the EmbedView tinymce plugin view component

862e7ac

Components: Reduxify the EmbedViewManager component

75737e7

Framework: Remove legacy flux embed stores

483bba6

Credits: Update embed normalization utility path

14dd01a

State: Mock normalizeEmbeds in reducer tests

bfac3f8

State: Use proper siteId in getEmbed tests

5aa4076

TinyMCE: Set default wrapper component state in EmbedView

112e4fc

Components: Add a QueryEmbed query component

6c4f534

TinyMCE: Use QueryEmbed in EmbedView

eb20a6f

TinyMCE: In EmbedViewManager, request embed patterns only once per site

2d1cd8d

TinyMCE: Convert EmbedViewManager back to an EventEmitter, add Redux …

98e973b

…store listeners

TinyMCE: Move the current site embeds helper in a method

59fd5fa

Components: Fix QueryEmbed README example

f6a1096

tyxla force-pushed the reduxify/embeds branch from b401429 to f6a1096 Compare February 5, 2020 11:55

tyxla added 3 commits February 5, 2020 14:03

Components: Simplify QueryEmbed by using hooks

d264692

State: Introduce a getCurrentSiteEmbeds selector

3892ac2

TinyMCE: Use getCurrentSiteEmbeds in EmbedView

0417db9

jsnajdr approved these changes Feb 5, 2020

View reviewed changes

tyxla merged commit 3a0e741 into master Feb 5, 2020

tyxla deleted the reduxify/embeds branch February 5, 2020 18:08

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 5, 2020

This was referenced Feb 5, 2020

State: Remove site change listeners middleware #39285

Merged

Post Editor: Update EmbedView to use createRef instead of string refs #39316

Closed

Reduxify: lib/embeds store #24146

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: Reduxify embeds #38608

Framework: Reduxify embeds #38608

tyxla commented Dec 30, 2019 •

edited

Loading

matticbot commented Dec 30, 2019

matticbot commented Dec 30, 2019 •

edited

Loading

delawski left a comment

tyxla commented Jan 2, 2020

jsnajdr Jan 6, 2020

tyxla Feb 3, 2020

jsnajdr Jan 6, 2020

tyxla Feb 3, 2020

jsnajdr Jan 6, 2020

tyxla Feb 3, 2020

jsnajdr Feb 4, 2020

tyxla Feb 7, 2020

jsnajdr Jan 6, 2020

tyxla Feb 3, 2020

jsnajdr Jan 6, 2020

tyxla Feb 3, 2020

jsnajdr Jan 6, 2020

tyxla Feb 3, 2020

tyxla Feb 7, 2020

jsnajdr Jan 6, 2020

tyxla Feb 3, 2020

tyxla Feb 5, 2020

tyxla commented Feb 3, 2020 •

edited

Loading

jsnajdr left a comment

jsnajdr Feb 4, 2020

tyxla Feb 5, 2020

jsnajdr Feb 4, 2020

tyxla Feb 5, 2020

jsnajdr Feb 4, 2020

tyxla Feb 5, 2020

jsnajdr Feb 5, 2020

tyxla commented Feb 5, 2020

jsnajdr left a comment


		this.sitesListener = this.updateSite.bind( this );
		EmbedViewManager.match = content => {

		import { getSelectedSiteId } from 'state/ui/selectors';
		import { SELECTED_SITE_SUBSCRIBE, SELECTED_SITE_UNSUBSCRIBE } from 'state/action-types';

Framework: Reduxify embeds #38608

Framework: Reduxify embeds #38608

Conversation

tyxla commented Dec 30, 2019 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Dec 30, 2019

matticbot commented Dec 30, 2019 • edited Loading

delawski left a comment

Choose a reason for hiding this comment

tyxla commented Jan 2, 2020

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

tyxla commented Feb 3, 2020 • edited Loading

jsnajdr 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

tyxla commented Feb 5, 2020

jsnajdr left a comment

Choose a reason for hiding this comment

tyxla commented Dec 30, 2019 •

edited

Loading

matticbot commented Dec 30, 2019 •

edited

Loading

tyxla commented Feb 3, 2020 •

edited

Loading