Make WordPress Core

Opened 4 years ago

Closed 12 months ago

#48654 closed task (blessed) (maybelater)

Consider a solution/endpoint to lazy-load scripts and styles

Reported by: youknowriad's profile youknowriad Owned by: spacedmonkey's profile spacedmonkey
Milestone: Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch
Focuses: javascript, rest-api, performance Cc:

Description

As we move towards more JavaScript-heavy UIs, the need to lazy-load assets for performance reasons is increasing.

The Gutenberg plugin has also an experimental feature allowing users to install blocks available in the block directory without leaving the editor or refreshing the page.

In all these situations we need to load assets (JavaScript scripts and stylesheets) from the server. The only thing the client knows is the "handle" of these scripts and styles. It doesn't know the dependencies of these scripts and styles which need to be loaded as well.

So, the requirements here are:

  • Given a script or style handle name, retrieve its dependencies. (Possibly an endpoint)
  • Load these dependencies in the client without duplication (possibly a task that will be tackled in a separate ticket)

Ideally, this endpoint would work for any script and style handle but historically WordPress users may have attached sensible data to their scripts (by calling localize_script or add_inline_script). A solution here should address that issue

Potential solutions:

  • whitelist have a flag similar to show_in_rest saying whether the script is whitelisted or not. This solution risk excluding way too much scripts and styles creating bad user experiences (no lazy load possible, no inline block installs...)
  • blacklist Potentially find a way to detect sensible scripts and styles with rules, for example by saying that all localized scripts are blacklisted by default unless authorized using a flag.
  • Other solutions I didn't think of :)

No matter the solution, if we're not able to load all the dependencies, the client should be notified (in the endpoint response maybe) in order to take the proper action in that case.

Attachments (2)

48654.diff (8.8 KB) - added by spacedmonkey 4 years ago.
48654.2.diff (11.4 KB) - added by spacedmonkey 4 years ago.

Download all attachments as: .zip

Change History (42)

@spacedmonkey
4 years ago

#1 @spacedmonkey
4 years ago

  • Keywords has-patch dev-feedback needs-unit-tests added
  • Version set to 4.7

In 48654.diff I made a first pass at the new REST APIs. This patch, does nothing to handle the security issues related to show on register scripts and styles. Just output the scripts and styles in a usable format.

This patch adds the following APIs.

/wp-json/wp/v2/scripts
/wp-json/wp/v2/styles

Both of these apis have the followings options.

/wp-json/wp/v2/scripts - List all registered scripts
/wp-json/wp/v2/scripts/<handle> - List a single script by handle.
wp/v2/scripts/?dependency=<handle> - List all dependencies of a given handle. This includes the handle itself and recursive dependencies.

{
   handle: "jquery-ui-core",
   src: "/wp-includes/js/jquery/ui/core.min.js",
   deps: [
     "jquery"
   ],
   ver: "1.11.4",
   args: 1,
   extra: [ ],
   textdomain: null,
   translations_path: null,
   url: "http://src.wordpress-develop.test/wp-includes/js/jquery/ui/core.min.js?ver=1.11.4",
   _links: {
     self: [
       {
         href: "http://src.wordpress-develop.test/wp-json/wp/v2/scripts/jquery-ui-core"
       }
     ],
     collection: [
       {
         href: "http://src.wordpress-develop.test/wp-json/wp/v2/scripts"
       }
     ],
     deps: [
       {
         href: "http://src.wordpress-develop.test/wp-json/wp/v2/scripts/?dependency=jquery-ui-core"
       }
     ]
  }
},

Does this look like a work format for you @youknowriad

#2 @youknowriad
4 years ago

Thanks @spacedmonkey That's cool.

Maybe we can start by putting this in a Github PR and try to start using it with the Block Directory Inserter. It would be a good real use-case to validate the endpoint. Of course, we need to handle the security issues as well somehow.

Pinging some folks that might be interested in this @noisysocks @tellyworth @talldanwp

This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-restapi by aduth. View the logs.


4 years ago

#5 @kadamwhite
4 years ago

This is perhaps a naive response, but could we utilize oEmbed at all for this purpose? The need to expose the full script dependency graph seems like overkill, if a plugin's been installed and activated in the background then I'd think it should be possible to treat a given script handle as an oEmbed target and to dynamically load scripts through that manner instead of manually orchestrating everything from the Gutenberg JS code.

#6 @youknowriad
4 years ago

@kadamwhite I'm not sure I understand what you propose? Can you clarify a little bit.

#7 @youknowriad
4 years ago

I guess you mean something like the current load-scripts.php that also loads the dependencies of the given script. This may work too, conceptually it's very similar to the endpoint proposed above as it exposes the same data but not having to orchestrate inline scripts concats in the client is a win.

@spacedmonkey
4 years ago

#8 @spacedmonkey
4 years ago

In 48654.2.diff I added a basic permission model.

Here are the basic permissions rules.

All core scripts and styles are set as public. All can be found in WP core so should be considered public.

All public script / styles for blocks, also public. These are designed to be displayed on the front end.

All editor script / styles for blocks, check if user has access to edit_posts. These are designed to be displayed on the editor.

All other script / styles check for manage_options.

This model is not perfect, but it to get the ball rolling.

#9 @azaozz
4 years ago

Been thinking that there "must be" a better way to do dependencies in modern JS than to try to reuse script-loader from PHP.

If not, some form of dependency tracking as part of Gutenberg perhaps makes most sense? Lazy-loading (known) scripts from js seems pretty simple (assuming it's async and doesn't look into loading order).

Scripts that are already loaded will be "tracked" in js, then having the API provide the URLs for particular handles so "missing" dependencies can be lazy-loaded seems like enough?

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


4 years ago

#11 @spacedmonkey
4 years ago

I have created a PR on the gutenberg project to make it easier to test in GB.

@youknowriad Can you take a look and see if it is something that might be useful to you.

#12 @azaozz
4 years ago

The "hard part" here is this:

Load these dependencies in the client without duplication.

This is not possible to do from PHP (script-loader). The loaded scripts will have to be tracked in js and excluded from loading again if listed as dependencies. However not sure if it's a good idea to duplicate the script-loader functionality in js.

Seems it would be better to use script-loader to "calculate" only the missing dependencies. This can be done by keeping a list of the already loaded scripts (handles) in js, and include it on all async requests to load more scripts.

Use case: Let's assume there are 100 registered scripts in script-loader (PHP).

  1. The Edit Post screen is loaded (including the block editor). This loads 50 scripts (enqueued plus dependencies). The loaded scripts "handles" are stored in a list in js.
  2. From the Edit Post screen the user installs a new plugin. The plugin registers 3 new scripts that have 5 dependencies each.
  3. In the install confirmation response the three new script handles are passed to the block editor.
  4. The block editor makes a request to get the URLs to load the 3 new scripts. With this request it also sends the list of already loaded scripts.
  5. In PHP script-loader now has 103 registered scripts 50 of which are already loaded. It "calculates" the dependencies of the 3 new scripts, flattens them, then removes the already loaded scripts, and returns the URLs to the scripts that need to be loaded (in the proper load order).

#13 @dufresnesteven
4 years ago

With regards to lazy-loading, it won't change much in terms of the Block Directory Package seeing that assets are only loaded after user interaction and therefore technically only loaded when necessary. However, this will definitely be useful as we are looking for a better way to retrieve new block assets.

I don't quite understand what the plan is for 'lazy-loading' the assets (maybe the discussion hasn't happened yet).

Do we plan on using this for all installed blocks maybe?

Case A:

  1. Page is loaded ( with critical dependencies )
  2. Block assets are lazily loaded with Priority
    • Priority 1:
      • Blocks used in post/page
    • Priority 2:
      • All the other blocks?

Case B:

  1. Page is loaded ( with critical dependencies )
  2. Loads assets for blocks used in post/page
  3. On Inserter open, load remain block assets.

Case C:

  1. Page is loaded ( with critical dependencies )
  2. Loads assets for blocks used in post/page
  3. Inserter opened
  4. On Inserter interaction (Maybe hovering on icon, viewing category, etc...), load block assets.

... many other strategies..

This ticket was mentioned in Slack in #core-restapi by spacedmonkey. View the logs.


4 years ago

#15 @TimothyBlynJacobs
4 years ago

  • Milestone changed from Awaiting Review to 5.5
  • Owner set to spacedmonkey
  • Priority changed from normal to high
  • Status changed from new to assigned

Milestoning this for 5.5 since it is of importance to the Block Directory Inserter.

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


4 years ago

This ticket was mentioned in PR #360 on WordPress/wordpress-develop by dd32.


4 years ago
#17

This PR adds a unique ID attribute to the main JS script loader generated <script> tags, and takes it a step further to also add an ID for the related <script> tags for inline JS/translations/parameters.

The original intention of this was to migrate https://github.com/WordPress/gutenberg/pull/22721 for the Block Directory, but I'm not sure it's actually used at all right now.

This change does aid in debugging and future lazy loading though, and brings ID parity to <script> tags that the style loader generated <link>/<style> tags have.

Trac ticket: https://core.trac.wordpress.org/ticket/48654

#18 @gziolo
4 years ago

I think we should land a patch from @dd32 as a first step.

#19 @spacedmonkey
4 years ago

For on the patch for the styles and scripts endpoints is going on in the gutenberg plugin - https://github.com/WordPress/gutenberg/pull/21244

#20 @gziolo
4 years ago

  • Keywords commit added; dev-feedback needs-unit-tests removed

#21 @gziolo
4 years ago

I updated workflow keywords for the patch from @dd32 that doesn't address this ticket fully.

#22 @TimothyBlynJacobs
4 years ago

In 48295:

Script Loader: Add id attributes to script assets.

This commit adds a unique ID attribute to script loader generated <script> tags as well as related <script> tags for inline JavaScript, translations, or parameters.

This is a first step in adding support for lazy loading scripts and styles, but for now is only used to assist in debugging generated output.

Props dd32, spacedmonkey.
See #48654.

#23 @TimothyBlynJacobs
4 years ago

  • Milestone changed from 5.5 to 5.6
  • Type changed from feature request to task (blessed)

Committed the first part of this, moving to 5.6 for the REST API endpoints.

#24 @LewisCowles
4 years ago

There is a little work on this from a plugin I've recently been "adjusting".

Problems seem to include that actual DOM can be generated that may be presentational rather than just styling and scripting / interactivity.

https://github.com/CODESIGN2/gutenberg-shortcode-preview-block

The bones of the work were from the WordPress team before Gutenberg became core, but barring some minor changes to JS API, the PHP side has not changed, to the point header and footer are misleadingly called js and style

Perhaps the HTML clean up (I think it may be called kses) could be used with a different rule-set to clear out non JS and non-style / link tags?

That (along with how it can put itself within a post) and the Gutenberg side of it may be of use, since I've removed the plugin from the directory, but found myself tinkering since.

#25 @LewisCowles
4 years ago

Btw, the other reason I mention is that it has tests for the PHP, and a Storybook for the JS that might help understanding the client -> server communication

Last edited 4 years ago by LewisCowles (previous) (diff)

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-js by mdmag. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-restapi by mdmag. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 years ago

#30 @helen
4 years ago

  • Keywords commit removed

Removing commit keyword while the next step is being worked on.

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

#33 @TimothyBlynJacobs
3 years ago

  • Priority changed from high to normal

Removing the priority high keyword as well. It was high priority for 5.5, but we shipped with a stopgap solution. So it is now a ticket we’d like to implement to improve how the Block Directory functions, but isn’t a pressing 5.6 issue.

#34 @TimothyBlynJacobs
3 years ago

Bumping to Future Release, this hasn't seen much activity in the past few months. If someone is interested in taking it on for 5.6, feel free to re-milestone.

#35 @hellofromTonya
3 years ago

  • Milestone changed from 5.6 to Future Release

Punting per @TimothyBlynJacobs' comment above

Bumping to Future Release, this hasn't seen much activity in the past few months. If someone is interested in taking it on for 5.6, feel free to re-milestone.

#36 @spacedmonkey
2 years ago

  • Focuses performance added

#37 @gziolo
2 years ago

We discuss on GitHub in https://github.com/WordPress/gutenberg/issues/36716 using ES Modules and Import Maps as a potential way to improve the experience around lazy loading scripts.

#38 @spacedmonkey
13 months ago

@gziolo @youknowriad

I am willing to work on this ticket and get the REST api into core. What is the progress of this ticket? What needs to be done to use this REST API?

#39 @youknowriad
13 months ago

Hey @spacedmonkey and thanks for proposing to work on this ticket.

At the time this ticket was created, import maps and es modules were in their infancy. Now, it's clearer to me that basically this ticket should be closed or repurposed to rely on es modules and import maps that way we embrace the web platform.

In other words, I think the solution to this is to revive the exploration I've done here https://github.com/WordPress/gutenberg/pull/34172

#40 @spacedmonkey
12 months ago

  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from assigned to closed

Closing this ticket out for now, it is seems unneeded. Marking as maybe later.

Note: See TracTickets for help on using tickets.