Bring back tree shaking #91

kienstra · 2023-03-06T19:25:59Z

Brings back Phil's image tree shaking. Phil did almost all of the work already.

Fixes part of #57 (Ensure assets save appropriately)

How to test

Create a new pattern
Add an Image block
Click 'Insert from URL'
Enter https://via.placeholder.com/640x350
Click 'Create Pattern'
Reload the browser
Expected: The image still renders (though this worked before):

In the Image block, click 'Replace'
Scroll to the right, and click the pencil:

Enter https://via.placeholder.com/640x351
Click 'Update Pattern'
Reload the browser
Expected: The image still displays:

Go to <active theme>/patterns/images/
Expected: There's a 640x351 image, but not 640x350:

wp-modules/pattern-data-handlers/pattern-data-handlers.php

This reverts commit d393f4b.

kienstra · 2023-03-07T17:11:08Z

wp-modules/pattern-data-handlers/pattern-data-handlers.php

-	// Spin up the filesystem api.
-	$wp_filesystem = \PatternManager\GetWpFilesystem\get_wp_filesystem_api();
-
+function tree_shake_theme_images( $wp_filesystem, $copy_dir ) {


Adding 2 parameters isn't good.

But it allows testing this function.

Mocking the $wp_filesystem global was really hard, I couldn't figure it out.

kienstra · 2023-03-07T17:26:46Z

wp-modules/pattern-data-handlers/pattern-data-handlers.php

@@ -401,7 +402,7 @@ function tree_shake_theme_images() {
 	}

 	// Before we take any action, back up the current images directory.
-	copy_dir( $images_dir, $backedup_images_dir );
+	call_user_func( $copy_dir, $images_dir, $backedup_images_dir );


In production, this line should work the same as before.

The copy_dir function is passed as $copy_dir.

Before, this line blocked testing tree_shake_theme_images().

I couldn't figure out a way around it.

Seems like a minor tradeoff to enable testing.

kienstra · 2023-03-07T17:31:40Z

wp-modules/pattern-data-handlers/tests/PatternDataHandlersTest.php

+			[
+				$this->get_fixtures_directory() . '/patterns/images/used.jpg',
+			],
+			$wp_filesystem->get_copied()


This tests that it tree shakes the unused image (not-used.jpg) out of fixtures/patterns/images/.

But it keeps the used image (used.jpg).

kienstra · 2023-03-07T21:19:00Z

wp-modules/editor/model.php

@@ -79,6 +81,8 @@ function save_pattern_to_file( WP_Post $post ) {
 	);

 	add_action( 'rest_after_insert_' . get_pattern_post_type(), __NAMESPACE__ . '\save_pattern_to_file' );
+
+	tree_shake_theme_images( get_wp_filesystem_api(), 'copy_dir' );


Before, tree_shake_theme_images() ran in update_pattern().

And that ran on post and meta updates.

Now, this only runs on post updates.

post_content should only change on post updates, and that's the only thing tree shaking applies to.

Maybe tree shaking should also run on delete_pattern(), but I like the simplicity 😄

It could be a nice touch to also run on delete_pattern imo — not a huge thing by any means, but I think it would make sense to destroy the asset along with the pattern file.

That's a good point.

Applied in 8534c8e

Awesome, this is working well! Images are deleted along with the pattern file as expected.

Thanks for testing this

kienstra · 2023-03-07T21:19:36Z

wp-modules/pattern-data-handlers/pattern-data-handlers.php

@@ -116,6 +116,8 @@ function format_pattern_data( $pattern_data, $file ) {
 		$pattern_data['description'] = translate_with_gettext_context( $pattern_data['description'], 'Pattern description', $text_domain );
 	}

+	opcache_invalidate( $file );


Thanks to Phil's suggestion, this was getting an outdated cached version of this file.

This fixed it in my local, let me know if this fixes it for you.

kienstra · 2023-03-08T21:41:23Z

Hi @johnstonphilip, does tree shaking work in your local with this?

mike-day

This seems to be working well, @kienstra!

Upon removing an image from a given pattern and saving the post, the image is removed from the images subfolder.

I am also not seeing any caching issues as I have in the past, though they previously would appear sporadically.

Really nice work rounding this one out! You are @johnstonphilip are both monsters!

kienstra · 2023-03-09T18:52:18Z

Hi @mike-day,
Thanks so much for testing this, and for you idea to run on delete_pattern(). Really appreciate your review!

johnstonphilip · 2023-03-09T20:07:10Z

Amazing! This is actually working really well now. Excellent work @kienstra

kienstra · 2023-03-09T20:28:48Z

Thanks so much, @johnstonphilip! You did the work, this just brings it back 😄

Bring back tree shaking

3b39cff

kienstra mentioned this pull request Mar 6, 2023

Beta task checklist #57

Open

11 tasks

kienstra commented Mar 6, 2023

View reviewed changes

wp-modules/pattern-data-handlers/pattern-data-handlers.php Outdated Show resolved Hide resolved

kienstra added 27 commits March 6, 2023 14:02

Add a very basic test for tree shaking

4b0b0da

Assert that there are 2 patterns

e26233f

Fix the name of the pattern in tests

727182b

Add a stub image that's not used

65d653a

Remove unused image link

ee80417

See if this can filter the filesystem

73ee08d

Add a missing comma

06df824

See if this can use WP_Filesystem_UnitTestCase

4dcb3b7

Make the tree shaking test simpler

b5bd167

Another guess at the test directory

bbf9c76

Move the tests directory one level back

998f1d1

Test move_block_images_to_theme()

c358209

Fix a syntax error

a2791e2

See if another directory works

9046c8d

See if the tests_dir is in /tmp

6c842e1

See if functions.php exists

64a8ac7

Another attempt at the test directory

b7c3165

Include the filesystem class

f287c38

Use dirname() to get the tests directory

ca89e06

Don't require the path anymore

f36d376

See if the tests/ directory is in this plugin

05dd678

Add echo statements for the tests_dir

d393f4b

Revert "Add echo statements for the tests_dir"

617141e

This reverts commit d393f4b.

Set the tests_dir to the one echoed

1202629

See if MockFS works

7a3b19c

Fix the use statement

94c5d4d

Hard code the WP_PHPUNIT__DIR

8dfc188

kienstra added 2 commits March 7, 2023 10:56

Remove extra phpcs:ignore comment

14db842

Remove the parameterized test

8403e79

kienstra commented Mar 7, 2023

View reviewed changes

kienstra added 2 commits March 7, 2023 11:19

Remove a file-wide disable

604c858

Also tree shake on pattern deletion

34819a3

kienstra commented Mar 7, 2023

View reviewed changes

kienstra added 5 commits March 7, 2023 11:35

Remove extra stub functions

abac6a0

Remove needless extra slash

4da3d92

Add wp_content_dir() back in

51edc34

See if ->find_folder() helps

7bad45d

Thanks to Phil's idea, invalidate the opcache for $file

bf3bb5f

kienstra force-pushed the add/tree-shaking-back-in branch from f93f30f to bf3bb5f Compare March 7, 2023 21:13

Add back in an empty line

f3f4dbe

kienstra commented Mar 7, 2023

View reviewed changes

kienstra marked this pull request as ready for review March 7, 2023 21:32

kienstra requested review from johnstonphilip, mike-day, mikemcalister and dreamwhisper March 7, 2023 21:48

Merge branch 'main' into add/tree-shaking-back-in

7bfb2b8

mike-day approved these changes Mar 9, 2023

View reviewed changes

Tree shake on deleting a pattern

8534c8e

kienstra merged commit 0a55171 into main Mar 9, 2023

kienstra deleted the add/tree-shaking-back-in branch March 9, 2023 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring back tree shaking #91

Bring back tree shaking #91

kienstra commented Mar 6, 2023 •

edited

Loading

kienstra Mar 7, 2023 •

edited

Loading

kienstra Mar 7, 2023 •

edited

Loading

mike-day Mar 9, 2023

kienstra Mar 9, 2023

kienstra Mar 7, 2023 •

edited

Loading

kienstra Mar 7, 2023 •

edited

Loading

kienstra Mar 7, 2023 •

edited

Loading

mike-day Mar 9, 2023

kienstra Mar 9, 2023

kienstra Mar 9, 2023

mike-day Mar 9, 2023

kienstra Mar 9, 2023

kienstra Mar 7, 2023 •

edited

Loading

kienstra commented Mar 8, 2023

mike-day left a comment

kienstra commented Mar 9, 2023

johnstonphilip commented Mar 9, 2023

kienstra commented Mar 9, 2023

Bring back tree shaking #91

Bring back tree shaking #91

Conversation

kienstra commented Mar 6, 2023 • edited Loading

How to test

kienstra Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

kienstra Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

mike-day Mar 9, 2023

Choose a reason for hiding this comment

kienstra Mar 9, 2023

Choose a reason for hiding this comment

kienstra Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

kienstra Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

kienstra Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

mike-day Mar 9, 2023

Choose a reason for hiding this comment

kienstra Mar 9, 2023

Choose a reason for hiding this comment

kienstra Mar 9, 2023

Choose a reason for hiding this comment

mike-day Mar 9, 2023

Choose a reason for hiding this comment

kienstra Mar 9, 2023

Choose a reason for hiding this comment

kienstra Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

kienstra commented Mar 8, 2023

mike-day left a comment

Choose a reason for hiding this comment

kienstra commented Mar 9, 2023

johnstonphilip commented Mar 9, 2023

kienstra commented Mar 9, 2023

kienstra commented Mar 6, 2023 •

edited

Loading

kienstra Mar 7, 2023 •

edited

Loading

kienstra Mar 7, 2023 •

edited

Loading

kienstra Mar 7, 2023 •

edited

Loading

kienstra Mar 7, 2023 •

edited

Loading

kienstra Mar 7, 2023 •

edited

Loading

kienstra Mar 7, 2023 •

edited

Loading