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

Cache JS dependencies loaded from external URLs #795

Merged
merged 5 commits into from
May 18, 2017

Conversation

nylen
Copy link
Member

@nylen nylen commented May 15, 2017

On slow connections especially, the Gutenberg editor takes a long time to load. The main reason for this is that we load 3MB of JavaScript from external sources. This PR adds code that attempts to cache these dependencies in local files and update them every day.

Fixes #647 (at least mostly). There are still a lot of network requests when loading the editor:

  1. Font stylesheet from Google (https://fonts.googleapis.com/css?family=Noto+Serif%3A400%2C400i%2C700%2C700i&ver=4.8-alpha-XXXXX-src)
  2. Images in demo content (notably https://cldup.com/E4PzNdrFSQ.jpg at 2MB)
  3. A whole bunch of stuff loaded by the YouTube embed in the demo content

We can probably get rid of (1) and (2) as well, but none of these remaining items block loading the page.

@nylen
Copy link
Member Author

nylen commented May 15, 2017

Also, for a future enhancement that can build on this PR: it would be good to use a similar mechanism to include these vendor scripts directly into the "production" plugin zip builds.

@mtias mtias added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Task Issues or PRs that have been broken down into an individual action to take labels May 16, 2017
@mtias
Copy link
Member

mtias commented May 16, 2017

Thanks for looking into this, the initial load is indeed problematic.

if ( ! $f ) {
// Failed to open the file for writing, probably due to server
// permissions. Enqueue the script directly from the URL instead.
// Note: the `fopen` call above will have generated a warning.
Copy link
Member

Choose a reason for hiding this comment

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

Would we want to suppress this via @fopen?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure how long this code will stick around. I don't have a strong preference either way, but if this is primarily in place during early development, this warning seemed potentially helpful for power users.

if ( $needs_fetch ) {
// Determine whether we can write to this file. If not, don't waste
// time doing a network request.
$f = fopen( $full_path, 'a' );
Copy link
Member

Choose a reason for hiding this comment

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

Can we just create a single file resource for the entirety of this function?

$f = fopen( $full_path, 'w' );
if ( ! $f ) {
	wp_register_script( $handle, $src, $deps );
	return;
}
$response = wp_remote_get( $src );
$is_success = wp_remote_retrieve_response_code( $response ) === 200;
if ( $is_success ) {
	fwrite( $f, wp_remote_retrieve_body( $response ) );
}
fclose( $f );
if ( ! $is_success ) {
	wp_register_script( $handle, $src, $deps );
	if ( ! filesize( $full_path ) ) {
		unlink( $full_path );
	}
	return;
}

Or would this potentially destroy an already-cached file if the update request is unsuccessful?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I had originally done something like this, but fopen( ..., 'w' ) truncates the file.

It might be possible to use 'r+' but I wanted to avoid having to truncate the file and move the pointer around because this seemed pretty error-prone.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Looks good and works well 👍

@nylen nylen merged commit 3dd7202 into master May 18, 2017
@nylen nylen deleted the add/mostly-offline-development branch May 18, 2017 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants