Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gutenberg: add oembed support #27486

Merged
merged 2 commits into from
Sep 28, 2018
Merged

Gutenberg: add oembed support #27486

merged 2 commits into from
Sep 28, 2018

Conversation

gwwar
Copy link
Contributor

@gwwar gwwar commented Sep 27, 2018

Fixes #27296 and alternative to #27437 and builds heavily on @notnownikki's earlier integration work.

This adds a middleware handler to transform the oembed/1.0 requests to be site aware!

screen shot 2018-09-27 at 4 14 32 pm

Testing Instructions

  • checkout this branch
  • Visit /gutenberg
  • Select a site
  • Once the editor loads try pasting in https://www.youtube.com/watch?v=kfVsfOSbJY0 or a tweet
  • We should see an embed preview, and if we publish it should be available.

💖 Props to @notnownikki for figuring out that we were missing a style causing iframes to be set to a width of 0

@gwwar gwwar added [Status] In Progress [Goal] Gutenberg Working towards full integration with Gutenberg labels Sep 27, 2018
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=add/oembed

@gwwar gwwar self-assigned this Sep 27, 2018
@gwwar gwwar 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 Sep 27, 2018
@gwwar gwwar requested a review from a team September 27, 2018 23:17
@gwwar gwwar added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 27, 2018
@gwwar gwwar 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 Sep 28, 2018
@gwwar gwwar force-pushed the add/oembed branch 3 times, most recently from cfc1bd7 to 2b63c05 Compare September 28, 2018 02:48
* @param {String} embedUrl the fallback embed url
* @returns {Object} transformed response
*/
const transformOembedResponseFromWpcomToCore = ( response, embedUrl ) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we could get an error here we should probably rename this maybeTransformOembedResponseFromWpcomToCore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah well that's one way to get me to refactor code :) updated in 445f074

Though I'll probably leave any polishing of error cases for follow ups

@mdawaffe
Copy link
Member

From #27296, it sounds like Gutenberg on Calypso is trying to access https://public-api.wordpress.com/wp/v2/oembed/1.0/proxy?url=…. It's not that that endpoint is not supported; the problem is that's not the right URL. I suspect some sort of Gutenberg+Calypso mapping code is misunderstanding the oembed/1.0 namespaced URLs.

The correct endpoint is https://public-api.wordpress.com/oembed/1.0/sites/:site/proxy?url=…. (the wp/v2 namespace and the oembed/1.0 namespace are separate.) That endpoint already works on that URL.

Naturally, there are some gotchas :)

  1. The response for WordPress.com posts is different from the response for Core WordPress posts (I think regardless of Jetpack status). WordPress.com posts return type=link and Core WordPress posts return type=rich. This isn't really a bug… different sites will output different sorts of oEmbed responses. It's just odd that our two, very similar ecosystems output different things. Resolving this difference will require cleaning up some not-so-superficial WordPress.com oEmbed code.
  2. It works for URLs of Jetpack sites (?url=https://some.jetpack.example.com/cool-post/), but the endpoint itself is not implemented in/forwarded to Jetpack sites. That is, /oembed/1.0/sites/some.jetpack.example.com/proxy?url=… will not (yet) work. It's not clear to me if we should forward the request to the Jetpack site, or just handle it locally on WordPress.com.

Both gotchas can be fixed, but are issues separate to the issue of just getting things working in Gutenberg. This PR (or whatever works for you all) is probably the fastest way forward for now. Hopefully future gotcha fixing will allow us to remove (or at least simplify) this middleware.

(PS: Sorry if this is all old news.)

@Copons
Copy link
Contributor

Copons commented Sep 28, 2018

https://www.youtube.com/watch?v=kfVsfOSbJY0

😡 😡 😡

@Copons
Copy link
Contributor

Copons commented Sep 28, 2018

I don't have enough knowledge of the middleware to give a really educated review, but this LGTM and it works as expected!

@gwwar
Copy link
Contributor Author

gwwar commented Sep 28, 2018

Going to rebase, so we can drop the diff d6f52db

@gwwar
Copy link
Contributor Author

gwwar commented Sep 28, 2018

The correct endpoint is https://public-api.wordpress.com/oembed/1.0/sites/:site/proxy?url=…. (the wp/v2 namespace and the oembed/1.0 namespace are separate.) That endpoint already works on that URL.

Thanks @mdawaffe for the clarification! We'll at least need some minimal middleware to update the path to be site aware. I'll give it a try! We'll need to test later if that endpoint already includes items on our wpcom +jetpack whitelist that should contain more embeds than core.

@notnownikki
Copy link
Contributor

Just a comment on the wp/v2 vs oembed/1.0 endpoints, something to make sure we test.

Gutenberg should work fine with both. However, if the 1.0 endpoint is the same as the proxy endpoint in core, then it might not work fully with all of Jetpack's embeds. Specifically, Pinterest.

Pinterest embed is implemented with a shortcode, and that shortcode enqueues scripts. The wp/v2 endpoint returns extra scripts metadata that Gutenberg uses to render the preview. In Gutenberg core, there's a wrapper to embed calls that logs any enqueued scripts and returns them in the same way that the wp/v2 endpoint does. Eventually, this wrapper will end up in WP core, but for now it's part of the plugin.

That's the only embed that I found has this behaviour at the moment, however that might be a factor in which endpoint we use. I noticed that Calypso's existing editor uses the wp/v2 endpoint for embed previews, which is why the gutenberg-wpcom fetch middleware routes embed requests there.

Hopefully that makes sense? Happy to work with all you lovely peeps on this :)

@gwwar
Copy link
Contributor Author

gwwar commented Sep 28, 2018

@mdawaffe looks like this behaves okay on the oembed/1.0 namespace from some brief testing. I'll land this once test are green, and I'll follow up with another issue to audit if we support extra wpcom/jetpack embeds that should be whitelisted. cc @notnownikki

@gwwar
Copy link
Contributor Author

gwwar commented Sep 28, 2018

Thanks for the reviews folks! ✨

@gwwar gwwar merged commit 0de1120 into master Sep 28, 2018
@gwwar gwwar deleted the add/oembed branch September 28, 2018 19:31
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Gutenberg Working towards full integration with Gutenberg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants