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

Add script and style REST APIs #21244

Open
wants to merge 45 commits into
base: trunk
Choose a base branch
from

Conversation

spacedmonkey
Copy link
Member

@spacedmonkey spacedmonkey commented Mar 29, 2020

Description

Create REST APIs for registered scripts and styles.

See original ticket at #48654

How has this been tested?

Endpoints can be found at
/__experimental/scripts and /__experimental/styles

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@jorgefilipecosta
Copy link
Member

cc: @StevenDufresne as I think this ticket may be important for the block directory work.

@StevenDufresne
Copy link
Contributor

Noting:

In order to make use of this endpoint in the Block Directory Package, we will need to update the code to install then add scripts. It currently adds scripts and tries to install.

Coincidentally, I have a PR open that does exactly that : #20001.

Co-Authored-By: Steven Dufresne <steve.dufresne@automattic.com>
@spacedmonkey
Copy link
Member Author

Worth noting this is WIP. It has no tests and only enough comments and formatting that travis passes. It really needs someone from GB team to look and maybe use before we continue with more PHP work. I don't want to spend time on a none starter.

@noahtallen
Copy link
Member

cc @ockham this relates to some stuff we've discussed related to more async loading of things in the editor

@gziolo gziolo added the REST API Interaction Related to REST API label Apr 27, 2020
@azaozz
Copy link
Contributor

azaozz commented Apr 28, 2020

As mentioned at #21684 (comment) I'm not sure how the REST API should be used together with core's Script Loader. Is the purpose to duplicate script loader in JS, or perhaps partially extend it, or..?

If the current Script Loader must be used, it seems a lot simpler to just add a function to it that will take a script's handle as param, run WP, calculate the dependencies, and return either the script tags (including inline scripts) or the concatenated JS as blob, or... anything else that may be useful. Of course that will need a list (array, CSV, etc.) of already loaded script handles that will be excluded from the calculated dependencies.

@spacedmonkey
Copy link
Member Author

@azaozz This REST API allows you to pass a script and style handle and it return an array of javascript / css files. From that, a script / style tag could easily be generated in gutenberg.

As for concatting javascript / css files. Little worried about this that jQuery are react might be reloaded unnecessarily. Feel like returning the data is more useful.

@spacedmonkey
Copy link
Member Author

Now that #21065 has been merge, I have updated the PR with embedded script and styles. Meaning that urls are for scripts can be requested as part of api request.

What you do think of this PR @TimothyBJacobs ?

@azaozz
Copy link
Contributor

azaozz commented May 29, 2020

This REST API allows you to pass a script and style handle and it return an array of javascript / css files. From that, a script / style tag could easily be generated in gutenberg.

Right, but an array of URIs to javascript files doesn't include all of the dependencies. There are also the "inline" dependencies (the js code added with add_inline_script() and wp_localize_script()) that would be missing.

For example, wp-mediaelement has four dependencies, two are in <script> tags and two are in *.js files. When wp-mediaelement is enqueued in script-loader, the resulting HTML is something like:

<script>
var mejsL10n = {"language":"en","strings":{"mejs.download-file":"Download File", ... }};
</script>
<script src='/wp-includes/js/mediaelement/mediaelement-and-player.js?ver=4.2.13-9993131'></script>
<script src='/wp-includes/js/mediaelement/mediaelement-migrate.js?ver=5.5-alpha-20200526.094621'></script>
<script>
var _wpmejsSettings = {"pluginPath":"\/wp-includes\/js\/mediaelement\/","classPrefix":"mejs-","stretching":"responsive"};
</script>
<script src='/wp-includes/js/mediaelement/wp-mediaelement.js?ver=5.5-alpha-20200526.094621'></script>

As far as I see currently there is no support for the JS code that goes in the two <script> tags. Only the URIs that go in <script src="..."> tags are supported.

Looking at how the inline scripts can be supported, there are generally two methods:

  • Each dependency could be an object that includes the before and after script code (when present). Then on the js side these chunks of js code will have to be run with eval().
  • All of the dependencies can be gathered and concatenated on the server (using file_get_contents(), etc.) then returned as one big chunk of js code. (In order for this to work, the already loaded scripts will have to be excluded from the request, or a list of the already loaded scripts will have to be passed to the server, so they can be excluded there).

@spacedmonkey
Copy link
Member Author

@azaozz Have you reviewed the output of the api?

Take this example from - /wp/v2/scripts/wp-ajax-response

{"handle":"wp-ajax-response","src":"\/wp-includes\/js\/wp-ajax-response.min.js","deps":["jquery"],"ver":false,"args":1,"extra":{"data":"var wpAjax = {\"noPerm\":\"Sorry, you are not allowed to do that.\",\"broken\":\"Something went wrong.\"};"},"textdomain":null,"translations_path":null,"url":"http:\/\/localhost:8889\/wp-includes\/js\/wp-ajax-response.min.js","_links":{"self":[{"href":"http:\/\/localhost:8889\/wp-json\/wp\/v2\/scripts\/wp-ajax-response"}],"collection":[{"href":"http:\/\/localhost:8889\/wp-json\/wp\/v2\/scripts"}],"deps":[{"href":"http:\/\/localhost:8889\/wp-json\/wp\/v2\/scripts\/?dependency=wp-ajax-response"}]}}

As you can see, the dependies are an listed as an array and all the extras are including, like translated strings.

This is all the data is required to rebuild the enqueue system in javascript.

@azaozz
Copy link
Contributor

azaozz commented May 30, 2020

Have you reviewed the output of the api?

Hmmmm, don't see this here? Will reset caches/npm and try again, sorry.

@azaozz
Copy link
Contributor

azaozz commented Jun 1, 2020

Got it to work, was my error, sorry :)

Couple of quick notes:

  1. Testing with https://site.tld/wp-json/wp/v2/scripts/wp-mediaelement the result is as expected:
handle: "wp-mediaelement"
src: "/wp-includes/js/mediaelement/wp-mediaelement.js"
...
url: "https://site.tld/wp-includes/js/mediaelement/wp-mediaelement.js"

However testing with https://site.tld/wp-json/wp/v2/scripts/mediaelement (which has no src as it is dependencies-only handle), the results are:

handle: "mediaelement"
src: false
...
url: "/wp-json/wp/v2/scripts/mediaelement?ver=4.2.13-9993131"

Wondering why the discrepancy for the url (seems it has to be empty, perhaps, when no src)?

  1. Testing with https://site.tld/wp-json/wp/v2/scripts/?dependency=wp-mediaelement (to get the handle plus all dependencies), the result is as expected.

There is a discrepancy with the extra depending on how the data was added (it's the same in PHP).

extra:
    data: "var _wpmejsSettings = ..."
handle: "mediaelement"
extra:
    before: Array(2)
        0: false
        1: "var mejsL10n = ..."
handle: "mediaelement-core"

Wondering if the API should be normalizing these in some way or at least removing the empty/invalid elements? On the server that data is not for "direct consumption", it's just a storage that shouldn't be accessed directly. Also note that plugins can add more data there, so an array is probably the proper format.

@azaozz
Copy link
Contributor

azaozz commented Jun 1, 2020

Another thing that needs fixing is to perhaps run the init and wp_enqueue_scripts actions in the API. Many plugins use them to enqueue/dequeue scripts and/or add data, etc. Also, perhaps may be a good idea to have a specific action only for the API dependencies.

@youknowriad
Copy link
Contributor

Thanks for your work here. I'm closing this PR now as we have import maps as an alternative now on Core.

Copy link

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @lukasz.pawlik@xwp.co.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: lukasz.pawlik@xwp.co.

Co-authored-by: spacedmonkey <spacedmonkey@git.wordpress.org>
Co-authored-by: lukaspawlik <lpawlik@git.wordpress.org>
Co-authored-by: dd32 <dd32@git.wordpress.org>
Co-authored-by: StevenDufresne <dufresnesteven@git.wordpress.org>
Co-authored-by: sarayourfriend <sarayourfriend@git.wordpress.org>
Co-authored-by: TimothyBJacobs <timothyblynjacobs@git.wordpress.org>
Co-authored-by: aristath <aristath@git.wordpress.org>
Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>
Co-authored-by: noahtallen <noahtallen@git.wordpress.org>
Co-authored-by: azaozz <azaozz@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REST API Interaction Related to REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.