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

Legacy widget rendering endpoint #1508

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Jul 19, 2021

Related to WordPress/gutenberg#33540
Trac Ticket: https://core.trac.wordpress.org/ticket/53957

This adds a /wp/v2/widget-types/<widget type>/render endpoint which accepts POST data and returns the same output as the previous iframe-oriented GET endpoint.

Test plan:

  1. Go to /wp-admin/widgets.php
  2. Paste the following script into your devtools:
wp.apiFetch({
      "path": "/index.php?rest_route=%2Fwp%2Fv2%2Fwidget-types%2Ftext%2Frender&context=edit&per_page=100&_locale=user",
      "method": "POST",
      "data": {"id_base":"text","instance":{"encoded":"YTo0OntzOjU6InRpdGxlIjtzOjU6InRlc3QyIjtzOjQ6InRleHQiO3M6NToidGVzdDMiO3M6NjoiZmlsdGVyIjtiOjE7czo2OiJ2aXN1YWwiO2I6MTt9","hash":"6bd53d501ad76cfbc50ef417d78dc9f1","raw":{"title":"test2","text":"test3","filter":true,"visual":true}}}
})
.then(console.log.bind(console))
.catch(console.error.bind(console));
  1. Confirm the response was successful and the widget got rendered as expected.

Alternatives considered:

  • ServerSideRender – we need to render a full HTML document with wp_head(), CSS links, maybe some additional scripts, and this is much more that I would like to return from a block rendering API.
  • Updating the existing admin_init hook handler to work with POST requests – it's tricky and doesn't seem right, I don't think we should stretch hooks like that. This clearly is an API request so let's treat it as such.

@adamziel adamziel marked this pull request as ready for review July 27, 2021 14:54
@adamziel
Copy link
Contributor Author

All the tests pass! @TimothyBJacobs – do you fancy a review?

@adamziel adamziel changed the title First stab at the legacy widget rendering endpoint Legacy widget rendering endpoint Aug 17, 2021
Copy link

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

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

Good job!
I only have 1 small remark.

@adamziel
Copy link
Contributor Author

Great suggestion @anton-vlasenko ! I just added it.

Copy link

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

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

Everything looks good 👍

@draganescu
Copy link

I think we should bring this in along with the change in the legacy widget that uses it.

@anton-vlasenko
Copy link

anton-vlasenko commented Aug 18, 2021

I think we should bring this in along with the change in the legacy widget that uses it.

@draganescu
I thought I need to wait until this PR is merged (I mean not this PR, but the corresponding Trac issue).
Sure, I will modify my PR to use the new REST endpoint.

@noisysocks
Copy link
Member

How will the iframe use this request? Can iframes make POST requests?

I'd also like to get the @TimothyBJacobs tick of approval before merging this 🙂

@kevin940726
Copy link
Member

@noisysocks I think the idea is to make that request using apiFetch and get the response as text then just feed it into the srcdoc prop of the iframe.

const response = await apiFetch({ path: '...' });
const html = await response.text();

<iframe srcDoc={html} />

In that case, I think this approach is very doable and could be backward-compatible and minimal.

@adamziel
Copy link
Contributor Author

adamziel commented Aug 19, 2021

@noisysocks exactly as @kevin940726 said. We may either set srcDoc or tamper with contentDocument (but I like the srcDoc more). Either way, the iframe wouldn only be a vessel for the data we get via apiFetch.

I think we should bring this in along with the change in the legacy widget that uses it.

@draganescu the change in the legacy widget depends on this change. We'll also have to polyfill this endpoint in the Gutenberg plugin for 5.7 and 5.8. Now that I think about it, maybe we should only add it to Gutenberg repo and not here 🤔 and then backport it on the next major release

@spacedmonkey
Copy link
Member

CC @TimothyBJacobs

@adamziel
Copy link
Contributor Author

@TimothyBJacobs thank you for reviewing! I just addressed all your feedback.

@TimothyBJacobs
Copy link
Member

Thanks @adamziel! The only small tweak is that the @since tags should come before the @param tags.

@noisysocks
Copy link
Member

noisysocks commented Aug 23, 2021

Last minute feedback, sorry!

Did you consider incorporating this functionality into the existing /widget-types/:id/encode endpoint? /encode is an existing RPC-style endpoint that already accepts instance as an argument and returns preview in the response. The only difference is that /encode does not output all of the wrapping HTML including scripts and styles and whatnot. Perhaps it could, though. I'm not sure that we're using the preview from /encode anywhere. If it's possible to have fewer endpoints (especially RPC-style ones) then I'd prefer that! 😀

@noisysocks
Copy link
Member

Now that I think about it, maybe we should only add it to Gutenberg repo and not here 🤔 and then backport it on the next major release

Yes we probably should do this. That way it can be used by WordPress/gutenberg#34184, too.

@adamziel
Copy link
Contributor Author

Let's continue in the Gutenberg repo then: WordPress/gutenberg#34230

@adamziel adamziel closed this Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants