Move embed API call out of block and into data #6678

notnownikki · 2018-05-10T08:33:19Z

Description

The removes the API call from inside the embed block and uses withSelect instead.

Moving the data requests into the framework and makes it possible to extend the embed behaviour through plugin code

How has this been tested?

The usual embed tests from the Sandbox README, plus doing them twice in a row to make sure the embed previews come from the store, and try to embed a url that cannot be embedded to make sure the correct error is reported.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

aduth · 2018-05-23T18:44:19Z

This surfaces a question: Is embed API data really block data? Or is it a property entity of the API that we should house within core-data ? If it's the former, how can we avoid redundancy with data entity tracking between blocks and core-data ? Would we want stores specific to an individual block, or for all blocks ? It's important to set expectations here for plugin authors who might have similar needs, and it's a question which I've pondered a bit on components (e.g. #5316, "Bring viewport into the components module, while still allowing it to declare its own store.").

notnownikki · 2018-05-25T14:47:07Z

Been thinking about this.

This is an API call. It's not specific to that block instance (it's totally possible to embed the same URL in multiple blocks)... is it specific to that block type? Are there other blocks that would use embed data? Are there other places in the editor?

Would we want stores specific to an individual block, or for all blocks ?

I'd think that we would want individual block stores if that block's state was stored in redux, rather than this.state. The line here is a bit blurry, because the data from the embed API does determine the embed block's editor state, but if you have two blocks embedding the same URL, then that API data shouldn't be duplicated.

With it being an API call, it does seem like core-data is a better place for it. I appreciate we don't want stores cluttering things up with random data from random APIs, and while it's not that likely to happen with embed blocks (usually you only embed a URL once...), it's conceivable that other blocks might want to fetch the same data from an API for multiple instances of that block.

I'm happy to change the store this uses. Perhaps we need to agree on a naming convention for API data fetched by blocks?

How do you think we should move forward?

aduth · 2018-05-29T18:57:03Z

I think we should move this to core-data. Not sure entirely what you mean about naming convention. As I see it, we don't consider this as within the scope of blocks at all. It's embed data from the REST API; that this particular block consumes it is incidental.

notnownikki · 2018-05-30T09:43:08Z

Ok, moving this into core data.

By "naming convention" I meant have a predictable name for where API data is going to be in the store, but if that's not really a concern at this point I'll just move it into core data as it is.

aduth · 2018-06-25T13:30:20Z

core-data/selectors.js

+ *
+ * @return {Mixed} Undefined if the preview has not been fetched, false if the URL cannot be embedded, array of embed preview data if the preview has been fetched.
+ */
+export function getPreview( state, url ) {


To disambiguate, could we name the selector getEmbedPreview? I might imagine this could be mistaken for e.g. a post preview.

aduth · 2018-06-25T13:31:10Z

core-data/selectors.js

+ * @param {Object} state    Data state.
+ * @param {string} url      Embedded URL.
+ *
+ * @return {Mixed} Undefined if the preview has not been fetched, false if the URL cannot be embedded, array of embed preview data if the preview has been fetched.


The correct form of the mixed type in JSDoc is {*}.

See: http://usejsdoc.org/tags-param.html#multiple-types-and-repeatable-parameters

However, it might be better to use the "nullable" type here.

http://usejsdoc.org/tags-type.html

This may not work so well given that it returns so many varying types, though this may identify that the selector is doing too much work. For example, should we consider a separate selector for isURLEmbeddable ?

aduth · 2018-06-25T13:33:18Z

core-data/selectors.js

+
+	const oEmbedLinkCheck = '<a href="' + url + '">' + url + '</a>';
+
+	if ( oEmbedLinkCheck === preview.html ) {


I wish there was a more reliable way to determine this from the server? Like a response property or header. Constructing a string URL to compare seems rather fragile.

aduth · 2018-06-25T13:36:08Z

core-blocks/embed/index.js

+					return;
+				}
+				setAttributes( { url } );
+				this.setState( { fetching: true, error: false } );


May be fine to address separately, but see isRequestingTerms in packages/core-data/src/selectors.js for an example of how we can rely on the resolution flow to determine whether a fetch is happening rather than using state.

notnownikki · 2018-06-25T13:41:31Z

@aduth thanks for those points, I'll address them when I fix the conflicts :)

notnownikki · 2018-06-28T12:05:39Z

Rebased on master and squashed commits.

Changed getPreview and receivePreview to getEmbedPreview and receiveEmbedPreview.
Corrected the mixed type to * in the JSDoc for getEmbedPreview

@aduth :

this may identify that the selector is doing too much work. For example, should we consider a separate selector for isURLEmbeddable ?

and...

May be fine to address separately, but see isRequestingTerms in packages/core-data/src/selectors.js for an example of how we can rely on the resolution flow to determine whether a fetch is happening rather than using state.

Yes, these two are related, we're relying on getEmbedPreview to return undefined if the fetch hasn't happened, so I agree, isRequestingEmbedPreview would be the right way forward. I'd like to do that as a seperate PR, if that's ok?

I wish there was a more reliable way to determine this from the server? Like a response property or header. Constructing a string URL to compare seems rather fragile.

I also wish that. It is extremely fragile, and I want to address this in another PR. We can have the embed_maybe_make_link filter always return false (maybe_make_link can only change the HTML, or return false if there is no fallback), which would mean any changes to the fallback HTML wouldn't affect the logic here. We'd need to do that just in the REST API, so that fallbacks would still apply on the published post... and that probably needs a discussion :)

aduth · 2018-07-10T14:59:37Z

core-blocks/embed/index.js

 				};
 			}

-			componentDidMount() {
-				this.doServerSideRender();
+			componentWillMount() {


componentWillMount is a deprecated lifecycle function, and can usually be substituted with constructor. Probably sensible anyways to set initial state via this.state.

https://reactjs.org/docs/react-component.html#the-component-lifecycle

aduth · 2018-07-10T15:00:01Z

core-blocks/embed/index.js

 			}

 			componentWillUnmount() {
 				// can't abort the fetch promise, so let it know we will unmount
 				this.unmounting = true;
 			}

+			componentWillReceiveProps( nextProps ) {


componentWillReceiveProps is a deprecated lifecycle function. For your purpose here, you might consider componentDidUpdate, though it will incur a secondary render unfortunately. getDerivedStateFromProps is another option, though will require more substantial refactor.

https://reactjs.org/docs/react-component.html#the-component-lifecycle

aduth · 2018-07-10T15:03:21Z

core-blocks/embed/index.js

+		edit: compose(
+			withSelect( ( select, ownProps ) => {
+				const { url } = ownProps.attributes;
+				// Preview is undefined if we don't know the status of it, false if it failed,


To me, false seems like a loose interpretation of a failed request, thus necessitating a comment like this explaining it. I might prefer just to have a separate selector here to determine success of the result.

notnownikki · 2018-07-23T14:50:17Z

@aduth I think I'd addressed your concerns here.

The deprecated lifecycle methods have been migrated to new, supported ones. There's a new selector to say if the preview we have is an oEmbed fallback or not, so no looking at undefined/false as having special meaning.

youknowriad

Globally looking good, I left some small comments, I'm mainly thinking about ways to simplify the Embed Block Edit component, It doesn't seem usefull to keep the state, unless I'm missing something?

youknowriad · 2018-08-02T15:21:57Z

packages/core-data/src/resolvers.js

+/**
+ * External dependencies
+ */
+import { stringify } from 'querystring';


you can use addQueryArgs instead of this. see #8300

youknowriad · 2018-08-02T15:24:21Z

packages/core-data/src/selectors.js

+	if ( ! preview ) {
+		return false;
+	}
+	return preview.html === oEmbedLinkCheck;


It seems fragile to test the html. WP can decide to change the fallback right? Can't we add an explicit attribute to the embed endpoint when it's a fallback: isFallback: true?

The embed fallbacks are currently done by a filter in embed_maybe_make_link, and that only deals with HTML, you can't change any of the data structure. We'd have to change the core embed code itself to do that.

youknowriad · 2018-08-02T15:24:53Z

packages/core-data/src/test/resolvers.js

+/**
+ * External dependencies
+ */
+import { stringify } from 'querystring';


same as above :)

youknowriad · 2018-08-02T15:28:37Z

core-blocks/embed/index.js

+			this.processPreview = this.processPreview.bind( this );
+
+			this.state = {
+				html: this.props.preview ? this.props.preview.html : '',


Can't we rely on the props directly and avoid all these state values now that the fetching is done in a HoC?

I hadn't noticed, but yes, we can get rid of a lot of the state!

We can easily get rid of everything except fetching and url! Which is nice :)

…w-fetched-with-data-api

youknowriad · 2018-08-03T10:35:06Z

core-blocks/embed/index.js

+
+			this.state = {
+				editingURL: false,
+				fetching: !! this.props.attributes.url && ! this.props.preview,


Can't we get rid of fetching using the isResolving selector in core/data?

I don't think so, because (if I'm reading this correctly) isResolving works on an attribute in a store, so every embed block on the page would go into fetching mode when one of them fetches.

@notnownikki No, it's not, you have to pass the same set of arguments passed to your getEmbedPreview selectors to the isResolving selector, so you'll pass the url of the current block which means you get the "fetching" status for the current selector call.

Oh then I had totally read it wrong! I'll get on that :)

…w-fetched-with-data-api

youknowriad · 2018-08-07T11:06:27Z

core-blocks/embed/index.js

 import { Button, Placeholder, Spinner, SandBox, IconButton, Toolbar } from '@wordpress/components';
 import { createBlock } from '@wordpress/blocks';
 import { RichText, BlockControls } from '@wordpress/editor';
-import apiFetch from '@wordpress/api-fetch';
-import { addQueryArgs } from '@wordpress/url';
+import { withSelect, isRequesting } from '@wordpress/data';


isRequesting here is useless right?

Yes, there are still some things to clean up, lint should have picked this up in the tests :)

youknowriad · 2018-08-07T11:09:17Z

core-blocks/embed/index.js

+			setAttributes( { url } );
+		}
+
+		processPreview() {


I'm not sure I understand what this function does as it seems to do a lot of different things. I wonder if it's better to remove it in place where needed? At least we should add a JS doc explaining the logic here.

youknowriad · 2018-08-07T11:09:55Z

core-blocks/embed/index.js

+					// Form has been submitted without changing the url, and we already have a preview,
+					// so process it again. Happens when the user clicks edit and then immediately
+					// clicks embed.
+					this.processPreview();


Do we really need to to do anything here? since the url stayed the same, why are we calling processPreview here?

This is one of the areas I need to clean up, because so much of the state isn't there any more. I'll mark this PR as WIP until I have everything cleaned up.

youknowriad

The changes are looking good to me 👍 One thing I was wondering though is that we don't have embed e2e test (switching blocks, preview ...) I think it's must because these involve some advanced workflows that are not tested at all. I'm approving because I think we can work on these separately.

notnownikki · 2018-08-07T13:31:05Z

Thank you!

I've been looking at how we can have e2e tests for these, the thing I'm running up against is that we'd be relying on YouTube etc. to return responses, at the moment we don't have an easy way of mocking these that I can see.

youknowriad · 2018-08-07T13:33:52Z

the thing I'm running up against is that we'd be relying on YouTube etc. to return responses, at the moment we don't have an easy way of mocking these that I can see.

I don't really care about mocking those, just take a public video/tweet anything. If it breaks once in a while because of network issues, it's not important, the most important thing is that it's tested :)

notnownikki · 2018-08-07T13:36:47Z

Great, I will start on that next!

notnownikki added the [Status] In Progress Tracking issues with work in progress label May 10, 2018

notnownikki self-assigned this May 10, 2018

notnownikki force-pushed the update/embed-preview-fetched-with-data-api branch from ba05e09 to 9e1e74d Compare May 10, 2018 15:39

notnownikki added [Feature] Blocks Overall functionality of blocks and removed [Status] In Progress Tracking issues with work in progress labels May 10, 2018

notnownikki force-pushed the update/embed-preview-fetched-with-data-api branch from 38f9cdd to d94f53a Compare May 16, 2018 13:23

notnownikki requested review from youknowriad and aduth May 16, 2018 15:01

notnownikki force-pushed the update/embed-preview-fetched-with-data-api branch from de3e458 to 9939995 Compare May 30, 2018 09:52

aduth reviewed Jun 25, 2018

View reviewed changes

notnownikki force-pushed the update/embed-preview-fetched-with-data-api branch from 067f54d to 5f26a75 Compare June 28, 2018 12:05

aduth reviewed Jul 10, 2018

View reviewed changes

notnownikki force-pushed the update/embed-preview-fetched-with-data-api branch from bf28e3e to 574a6f7 Compare July 23, 2018 15:35

notnownikki added 5 commits July 31, 2018 10:57

Move embed API call out of block and into data

1c5090e

Update for apiFetch

a78196b

fix tests

c9d9bc5

Move the edit component out of settings for easier testing

e2f4f0a

Reinstate edit controls

14b7e75

notnownikki force-pushed the update/embed-preview-fetched-with-data-api branch from 574a6f7 to 14b7e75 Compare July 31, 2018 10:28

Merge branch 'master' into update/embed-preview-fetched-with-data-api

49ef894

youknowriad reviewed Aug 2, 2018

View reviewed changes

notnownikki added 2 commits August 3, 2018 09:44

switch to addQueryArgs

3db8a04

Get rid of a lot of the embed edit component's state

796bf22

notnownikki added 2 commits August 3, 2018 10:36

Merge remote-tracking branch 'origin/master' into update/embed-previe…

320a2ec

…w-fetched-with-data-api

Fixed editing url state

741b2af

youknowriad reviewed Aug 3, 2018

View reviewed changes

notnownikki and others added 4 commits August 6, 2018 11:53

Merge remote-tracking branch 'origin/master' into update/embed-previe…

5b456c4

…w-fetched-with-data-api

Fix previous merge mess-up

35db192

Trying to make fetching happen

1a451d8

Fix isResolving core data selector

ad831ec

youknowriad reviewed Aug 7, 2018

View reviewed changes

notnownikki added the [Status] In Progress Tracking issues with work in progress label Aug 7, 2018

Split processPreview into methods with distinct responsibilities

142d263

youknowriad approved these changes Aug 7, 2018

View reviewed changes

notnownikki merged commit 7d8e27e into master Aug 7, 2018

youknowriad deleted the update/embed-preview-fetched-with-data-api branch August 7, 2018 14:12

mtias added this to the 3.5 milestone Aug 9, 2018

dependabot-preview bot mentioned this pull request Dec 31, 2019

Bump @wordpress/i18n from 2.0.0 to 3.7.0 wearerequired/rest-likes#97

Closed

dependabot-preview bot mentioned this pull request Feb 4, 2020

Bump @wordpress/jest-console from 3.4.0 to 3.5.0 tdreyno/druyan#215

Merged

dependabot-preview bot mentioned this pull request Apr 15, 2020

Bump @wordpress/babel-plugin-makepot from 3.4.0 to 3.5.0 swissspidy/gutenberg-i18n-block#93

Merged


		const oEmbedLinkCheck = '<a href="' + url + '">' + url + '</a>';

		if ( oEmbedLinkCheck === preview.html ) {

Move embed API call out of block and into data #6678

Move embed API call out of block and into data #6678

Conversation

notnownikki commented May 10, 2018 • edited Loading

Description

How has this been tested?

Checklist:

aduth commented May 23, 2018

notnownikki commented May 25, 2018

aduth commented May 29, 2018

notnownikki commented May 30, 2018

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

notnownikki commented Jun 25, 2018

notnownikki commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notnownikki commented Jul 23, 2018

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

youknowriad left a comment

Choose a reason for hiding this comment

notnownikki commented Aug 7, 2018

youknowriad commented Aug 7, 2018

notnownikki commented Aug 7, 2018

notnownikki commented May 10, 2018 •

edited

Loading