Lazy Images: Move into a package #16657

davidlonjon · 2020-07-31T07:07:36Z

Changes proposed in this Pull Request:

Moves the Lazy Images module into a package. @gravityrail should be able to provide more context. It has become a requirement for an (yet) undisclosed A8C project.

Does this pull request change what data or activity we track or use?

No it should not.

Testing instructions:

Go to wp-admin/admin.php?page=jetpack_modules
Activate the "Lazy images" module
Create a new post with an image inside it.
Open that post as a non logged in user
Look at the source of the page and verify that the img tag as the data-lazy-loaded="1" property.

Proposed changelog entry for your changes:

Moves the Lazy Images module into a package.

Additional development context:

Here are some additional information related to the decision I have made for migrating the Lazy Images module into a standalone package. I have made some assumptions which might be incorrect and therefore some changes will need to be amended based on your guidance.

While migrating the module to a package, I tried to remove or update references to the module source file in order place of the repository such as from:
- .eslintignore
- modules/photon-cdn/jetpack-manifest.php: I am not sure if this one would be generated automatically so i just updated the path to the JavaScript files manually.
- phpunit.xml.dist
- modules/lazy-images.php: I kept this file even though the module source moved into a package as I wanted to make sure the module pattern activation/deactivation still work. I have also moved some of the credit comments into the package itself (packages/lazy-images/src/lazy-images.php) as it seems appropriate to belong there as the package can be a standalone download in any project.
In packages/lazy-images/src/lazy-images.php:
- I have used the version 8.8 in the PHPDoc @since entry but I am unsure if this would be the correct version.
- I have created a new method called is_amp_request because as a standalone package it cannot rely anymore on the Jetpack_AMP_Support::is_amp_request() method.
- I have created a new method called get_file_url_for_environment because as a standalone package it cannot rely anymore on the Assets::get_file_url_for_environment method as the JS asset is located within the package.
- I have fixed the coding standards.
In packages/lazy-images/tests/php/test_class.lazy-images.php:
- I have fixed the coding standards.
In packages/lazy-images/src/js/lazy-images.js:
- I have fixed a spelling mistake
- I have fixed Fix an issue with a comma being in place of a semi-column ( see commit 746e75e)
I have introduced some tooling for building assets, running unit tests and linters. The reason is because I was not sure if/how to integrate these within the Jetpack workflow. So these changes might need to be altered.
- Previously the minified version of the lazy-images.js file was generated by the Jetpack build workflow. Here I have introduced a Webpack config to build the minified version. The yarn build command allows building the minified version of the JavaScript.
- Because the PHP test class (packages/lazy-images/tests/php/test_class.lazy-images.php) extends the WP_UnitTestCase class, it seems that the wordpress-tests-lib is required. Therefore I also needed to have a bootstrapped WordPress testing environment which I am creating with Docker. The yarn test:phpunit command allows to run the PHP unit tests.
- I have introduced linting the PHP code. The yarn php:lint command allows to run the lint the PHP code base.

As I mention some of my assumptions and decisions above might be incorrect and will need to be adapted to an existing Jetpack workflow.

jetpackbot · 2020-07-31T07:11:25Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-16657

Scheduled Jetpack release: September 1, 2020.
Scheduled code freeze: August 25, 2020

Generated by 🚫 dangerJS against ff30622

jeherve

Tests appear to be failing at the moment. Do you think you could take a look?

I wonder if it may be possible to switch to relying on something like WorDBless instead of having to set up WP in here?

modules/photon-cdn/jetpack-manifest.php

jeherve · 2020-07-31T07:52:29Z

packages/lazy-images/.phpcs.xml.dist

@@ -0,0 +1,36 @@
+<?xml version="1.0"?>


Do we need this custom configuration here? Can we rely on the existing configuration instead?

Looking more into this, I now understand better the processes on Jetpack :) I have now removed it so the existing configuration will be use instead.

jeherve · 2020-07-31T07:54:04Z

packages/lazy-images/webpack.config.prod.js

@@ -0,0 +1,12 @@
+const path = require("path");


I wonder if we should leave this out of the package, and handle file minification from the Jetpack plugin instead?

Agreed, let's do that and keep the build scripts out of the package.

I have created a new webpack config for packages at the root of the repo and added a new command to the package.json. Hopefully this is closer to what it should be. The webpack config for packages can be extended later if required i guess?

…ck build process

davidlonjon · 2020-08-04T14:36:23Z

Thanks for the feedback @jeherve.

I have addressed all of your feedback except for the failure of the unit tests.

Today I tried to use WorDBless and all the tests would work with it except for test_wp_get_attachment_image_gets_lazy_treatment and test_wp_get_attachment_image_does_not_get_lazy_treatment_when_skip_lazy_added

These 2 tests are making use of wp_get_attachment_image, wp_get_attachment_image_srcset or the gallery shortcode for which their output are being tested. I am not sure to see how it can be tested by only using WorDBless and not the create_upload_object to saves an attachment in the DB. I guess we could mock these but it would defeat the purpose.
The test are still failing with my current setup bootstrapping WordPress into docker but now it it is only a problem with the DB not being ready in time which I think I will be able to fix soon. If you have any suggestions/approach, please let me know.

…ic/jetpack into add/jetpack-lazy-images-package

gravityrail · 2020-08-17T21:44:20Z

@davidlonjon @leogermani I am still getting test failures after merging master:

Travis CI command: phpunit
Running PHP:Compatibility checks:
PHP Compatibility command: `composer php:compatibility .` 
PHP_CodeSniffer version 3.5.5 (stable) by Squiz (http://www.squiz.net)
Could not scan for classes inside "/home/travis/build/Automattic/jetpack/vendor/automattic/jetpack-lazy-images/src/" which does not appear to be a file nor a folder
> vendor/bin/phpcs -p -s --runtime-set testVersion '5.6-' --standard=PHPCompatibilityWP --ignore=docker,tools,tests,node_modules,vendor,packages/*/wordpress --extensions=php '.'
ERROR: the "PHPCompatibilityWP" coding standard is not installed. The installed coding standards are PSR12, PEAR, Zend, PSR1, MySource, PSR2 and Squiz
Script vendor/bin/phpcs -p -s --runtime-set testVersion '5.6-' --standard=PHPCompatibilityWP --ignore=docker,tools,tests,node_modules,vendor,packages/*/wordpress --extensions=php handling the php:compatibility event returned with error code 3
The command "./tests/run-travis.sh" exited with 1.

gravityrail · 2020-08-17T21:46:20Z

Actually there are even more different issues, if you click around the test failures. At least a few seem unrelated to these changes and should be fixed in master.

…n output does not get the lazy treatment when lazy images feature is skipped

davidlonjon · 2020-08-18T09:31:37Z

First, thank you so much @leogermani for the work you have done with WorDBless and as a result refactoring one of the tests here. This is awesome!

Regarding the test_wp_get_attachment_image_does_not_get_lazy_treatment_when_skip_lazy_added, I think you are right and the test is really similar from test_wp_get_attachment_image_gets_lazy_treatment. I kept them those 2 tests as they were already there.

But your comment actually prompted me to look again at it and since we only want to test the wp_get_attachment_image function output in the context that it does not get the get the lazy treatment when lazy images feature is skipped using the skip-lazy class, then I remove the use of the gallery shortcode and just tested the wp_get_attachment_image function directly. It seems to work so now there is no dependency on the gallery shortcode and the calls to wp_posts.

@gravityrail, regarding the tests failure you've seen. It turns out the reason was that the composer.lock was out of date. It took me a long while to figure it out by testing all the Travis build process locally double checking on different branches.

Finally all the tests are now passing again and it seems the integration works well enough again. Hopefully this is now it and we should be in a state that it could be merged?

cc @jeherve

leogermani

LGTM

changes applied

leogermani · 2020-08-18T11:47:23Z

Seems ready to go. @jeherve just confirm your request change was addressed.

gravityrail

LGTM, thanks for all your work on the tests

* master: (23 commits) Premium Blocks: set blocks availability (#16898) Compat Package: Fix method declaration compatibility (#16900) Jetpack Dashboard: More meaningful error notices. (#16883) Connection REST API: Unit test for the `remote_authorize` request. (#16879) use blog token to request jetpack.updateBlog (#16698) Improve Story block media loading (#16663) Simplify error notices for broken connections (#16655) Use new heartbeat package (#16285) wrap-paid-block: remove component. deprecated. (#16895) Social Previews: improve preview description handling (#16889) Stats module use blog token (#16727) Form Block: add a new Consent Field, a new Newsletter setting, and a new newsletter variation (#16808) AAG: Backup card, fall back to VP content in case of /rewind API error. (#16867) Donations: Fix dependencies (#16892) Creative Mail: update option to lowercase (#16861) Premium Blocks: Implement the new design (#16611) Requests to Stats CSV use the blog token (#16716) Update spacing around sharing buttons to avoid no bottom margin below the customize link. (#16811) Jetpack SSO: Cleaning up the `requestNonce` API request. (#16830) Donations: Update plans when currency changes (#16844) ...

davidlonjon · 2020-08-20T07:40:29Z

I pushed a commit merging master back into this branch to fix a merge conflict with the package-lock.json file. All seems good and the Travis CI build is passing again

leogermani

Approving again after rebase

jeherve · 2020-08-24T08:43:18Z

Internal reference: D48486-code

Co-authored-by: Daniel Walmsley <[email protected]> Co-authored-by: leogermani <[email protected]>

davidlonjon added 11 commits July 31, 2020 09:47

Migrate the Lazy Images module into a standalone package

5839699

Fix an issue with a comma being in place of a semi column

746e75e

Fix a spelling mistake

a18605c

Fix coding standards

d4618de

Fix coding standards

f0cc1b0

Fix spelling mistake

33ea52d

Fix require path

f467b76

Fix methods visibility

5cf899f

Remove irrelevant PHPDoc comment

acddf4e

Setup tooling for linting and running unit tests

07527ff

Update README

355d5e2

matticbot added the Touches WP.com Files label Jul 31, 2020

davidlonjon requested a review from gravityrail July 31, 2020 07:07

davidlonjon self-assigned this Jul 31, 2020

github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Jul 31, 2020

davidlonjon added the [Status] Needs Review This PR is ready for review. label Jul 31, 2020

jeherve added [Focus] Jetpack DNA [Feature] Lazy Images labels Jul 31, 2020

jeherve requested changes Jul 31, 2020

View reviewed changes

jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Jul 31, 2020

davidlonjon added 7 commits August 4, 2020 22:25

Move the build of the Lazy Images JavaScript assets to the core Jetpa…

0df3db6

…ck build process

Remove logic related to PHPCS checks

ec5dbf8

Move the commands to run PHP unit tests into the composer.json file

8ca53cf

Update README

1530be8

Revert back the jetpack manifest file as it is auto generated

d48c196

Add a missing composer install command

37bef2c

Add missing @sign

9b89229

Merge branch 'add/jetpack-lazy-images-package' of github.com:Automatt…

cba84cf

…ic/jetpack into add/jetpack-lazy-images-package

leogermani and others added 4 commits August 17, 2020 19:56

Merge branch 'master' into add/jetpack-lazy-images-package

5044142

Update composer.lock file

0dfe31d

Fix namespace path

ca1ae14

Refactor unit test to assert that the wp_get_attachment_image functio…

96657cd

…n output does not get the lazy treatment when lazy images feature is skipped

leogermani previously approved these changes Aug 18, 2020

View reviewed changes

leogermani added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Aug 18, 2020

gravityrail previously approved these changes Aug 18, 2020

View reviewed changes

davidlonjon dismissed stale reviews from gravityrail and leogermani via ff30622 August 20, 2020 00:47

leogermani approved these changes Aug 20, 2020

View reviewed changes

jeherve approved these changes Aug 24, 2020

View reviewed changes

jeherve merged commit e3b90f4 into master Aug 24, 2020

jeherve deleted the add/jetpack-lazy-images-package branch August 24, 2020 08:12

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 24, 2020

github-actions bot added this to the 8.9 milestone Aug 24, 2020

jeherve removed the [Status] Needs Changelog label Aug 25, 2020

jeherve mentioned this pull request Aug 25, 2020

Lazy Images: consolidate package to rely on Assets package #16971

Closed

pereirinha pushed a commit that referenced this pull request Sep 10, 2020

[not verified] Lazy Images: Move into a package (#16657)

980120a

Co-authored-by: Daniel Walmsley <[email protected]> Co-authored-by: leogermani <[email protected]>

kraftbj mentioned this pull request Oct 21, 2020

Lazy Images: Improve JS handling #17562

Closed

Lazy Images: Move into a package #16657

Lazy Images: Move into a package #16657

Uh oh!

Conversation

davidlonjon commented Jul 31, 2020

Changes proposed in this Pull Request:

Does this pull request change what data or activity we track or use?

Testing instructions:

Proposed changelog entry for your changes:

Additional development context:

Uh oh!

jetpackbot commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeherve Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

davidlonjon Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

jeherve Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

gravityrail Aug 1, 2020

Choose a reason for hiding this comment

Uh oh!

davidlonjon Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

davidlonjon commented Aug 4, 2020

Uh oh!

gravityrail commented Aug 17, 2020

Uh oh!

gravityrail commented Aug 17, 2020

Uh oh!

davidlonjon commented Aug 18, 2020

Uh oh!

leogermani left a comment

Choose a reason for hiding this comment

Uh oh!

leogermani commented Aug 18, 2020

Uh oh!

gravityrail left a comment

Choose a reason for hiding this comment

Uh oh!

davidlonjon commented Aug 20, 2020

Uh oh!

leogermani left a comment

Choose a reason for hiding this comment

Uh oh!

jeherve commented Aug 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jetpackbot commented Jul 31, 2020 •

edited

Loading