Lazy Images: Use calypso-build to build #17571

ockham · 2020-10-21T20:36:14Z

Fixes #17562 -- or rather, most of it:

us[e] calypso-build (which takes care of transpilation, and by default drops files into a separate build/ [edit: should be dist/] directory, thus resolving items 1. and 2.)

delet[e] built files from GH, and adding the [dist/] subdirectory to .gitignore in order to prevent that from happening in the future

Revert Build: Remove lazy-images ES5 validation #17561 to re-enable validation.

One item from #17562 isn't addressed by this PR:

add the Interaction Observer npm to package.json (rather than copying it verbatim)

I haven't yet found a really good way to do that, as we also need to build it, and include it as an external. We can address that in a subsequent iteration however.

Changes proposed in this Pull Request:

Use calypso-build to build the lazy-images package.

Jetpack product discussion

N/A

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

No

Testing instructions:

Run yarn build-production-packages, and verify that it passes (it includes ES5 validation!).
Verify that the Lazy images module still works (specifically, the built JS).

(I've tested a post with an image and the Lazy Images setting switched on and it seemed to work fine, but I'm not familiar enough with lazy images to be 100% certain that it's still working as before.)

Proposed changelog entry for your changes:

Use calypso-build to build the lazy-images package.

Note

In comments on #17562, it was brought up that we might need to include the built js files in GH in order for composer to be able to export this package. This would mean adding the dist/ directory. Should we do that? Or can composer include a build step for JS?

jetpackbot · 2020-10-21T20:42:05Z

Scheduled Jetpack release: December 1, 2020.
Scheduled code freeze: November 23, 2020

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.

Generated by 🚫 dangerJS against 1d7ffab

ockham · 2020-11-02T19:15:42Z

Planning to pick this up tomorrow!

ockham · 2020-11-06T12:52:28Z

Should be ready for review.

brbrr · 2020-11-06T16:35:27Z

package.json

@@ -19,13 +19,13 @@
 		"build-client": "gulp",
 		"build-concurrently": "yarn install-if-deps-outdated && yarn clean && yarn concurrently 'yarn build-client' 'yarn build-php' 'yarn build-extensions' 'yarn build-search' 'yarn build-packages'",
 		"build-extensions": "webpack --config ./webpack.config.extensions.js",
-		"build-packages": "webpack --config ./webpack.config.packages.js",
+		"build-packages": "cd packages/lazy-images/ && npm run build && cd ../../",


Any reason we use npm instead of yarn?

Also the cd ../../ shouldn't be necessary, yarn does its thing in a subshell.

If that's future-proofing for potentially having to build more packages in the future, it might be best to future-proof a little further and put the commands in a build script in bin/.

Any reason we use npm instead of yarn?

Oversight on my part, apologies. I'll change to npm 👍 Thanks for spotting 👍

Also the cd ../../ shouldn't be necessary, yarn does its thing in a subshell.

Good point, I'll remove it 👍

If that's future-proofing for potentially having to build more packages in the future, it might be best to future-proof a little further and put the commands in a build script in bin/.

I did it indeed for future-proofing! If we end up building more packages like this, I'd prefer using lerna, much like we do in Calypso, since it allows running the same command across a number of packages. (That would also allow us dropping the cd packages/lazy-images/ here.) See e.g. https://github.com/Automattic/wp-calypso/blob/8b3ef578098c061eb6838c516761822908e911cb/package.json#L95, which basically runs the prepare script across all packages listed in lerna.json.

Aaand f9c48b8 to use yarn instead of npm run.

anomiex · 2020-11-06T17:20:18Z

package.json

@@ -19,13 +19,13 @@
 		"build-client": "gulp",
 		"build-concurrently": "yarn install-if-deps-outdated && yarn clean && yarn concurrently 'yarn build-client' 'yarn build-php' 'yarn build-extensions' 'yarn build-search' 'yarn build-packages'",
 		"build-extensions": "webpack --config ./webpack.config.extensions.js",
-		"build-packages": "webpack --config ./webpack.config.packages.js",
+		"build-packages": "cd packages/lazy-images/ && npm run build && cd ../../",


Also the cd ../../ shouldn't be necessary, yarn does its thing in a subshell.

If that's future-proofing for potentially having to build more packages in the future, it might be best to future-proof a little further and put the commands in a build script in bin/.

anomiex · 2020-11-06T17:27:00Z

packages/lazy-images/package.json

+	},
+	"homepage": "https://github.com/Automattic/jetpack",
+	"scripts": {
+		"clean": "npx rimraf dist",


What does this do that "rm -rf dist" doesn't?

Other than download and run code from the Internet, of course.

It's cross-platform, so it should also work on Windows -- at least that's why we're using it in Calypso, which is where I copied it from 🙂

I'm not sure how much of Jetpack's stuff is intended to be run on Windows, versus gets run inside a Linux docker container anyway. I do see that the "clean" scripts in Jetpack's package.json all just use rm -rf.

If you are going to use a node thing, IMO it would be better to just declare it as a dependency instead of having npx download an arbitrary version at runtime.

Happy to use rm -rf, if that's what JP does for everything else 👍

anomiex · 2020-11-06T17:30:32Z

packages/lazy-images/package.json

+	"scripts": {
+		"clean": "npx rimraf dist",
+		"prebuild": "yarn run clean",
+		"build": "calypso-build lazy-images='./src/js/lazy-images.js' intersectionobserver-polyfill='./src/js/intersectionobserver-polyfill.js'"


Should the dependency on @automattic/calypso-build be declared below, so this works standalone in addition to when being called from ../../package.json?

Yeah, makes sense! I'll add it as a devDependency.

packages/lazy-images/package.json

ockham · 2020-11-06T18:12:31Z

Just noticed that intersection-observer doesn't need transpilation. Will copy it to dist/ instead.

ockham · 2020-11-06T19:25:58Z

Thanks @brbrr and @anomiex for reviewing! I think I've addressed all feedback. One remaining question:

In comments on #17562, it was brought up that we might need to include the built js files in GH in order for composer to be able to export this package. This would mean adding the dist/ directory. Should we do that? Or can composer include a build step for JS?

anomiex

In comments on #17562, it was brought up that we might need to include the built js files in GH in order for composer to be able to export this package. This would mean adding the dist/ directory. Should we do that? Or can composer include a build step for JS?

Composer itself probably can't include a build step for JS. It seems to use whatever you get from e.g. https://github.com/Automattic/jetpack-lazy-images/archive/v1.2.0.zip. Can you have a build step for those Github zip downloads? Or can whatever mirrors the code from this monorepo into https://github.com/Automattic/jetpack-lazy-images/ run your build step? I don't know.

Or can we do release branches that have the files checked in even if they're not checked into master, and live with the composer package not working if someone tries to use "dev-master" or anything else that's not one of those release branches?

Note that you're also depending on node_modules/intersection-observer/intersection-observer.js, so that would also have to be checked into the repo if you can't figure out how to do a build step.

anomiex · 2020-11-06T21:20:37Z

packages/lazy-images/.gitignore

@@ -1,3 +1,5 @@
+dist
+node_modules
 vendor
 wordpress
 composer.lock


Looks like yarn creates a "yarn.lock" file that should probably also be ignored.

ockham · 2020-11-09T11:51:18Z

In comments on #17562, it was brought up that we might need to include the built js files in GH in order for composer to be able to export this package. This would mean adding the dist/ directory. Should we do that? Or can composer include a build step for JS?

Composer itself probably can't include a build step for JS. It seems to use whatever you get from e.g. https://github.com/Automattic/jetpack-lazy-images/archive/v1.2.0.zip.

Ah, I wasn't even aware of that repo, thanks for bringing that up!

Can you have a build step for those Github zip downloads?

We can have something like that, yeah: A GitHub action that's triggered by adding a new tag and then runs the build step and creates a release with the resulting zip attached as an asset. (Here's an example of what that looks like: WordPress/gutenberg#19626 and https://github.com/ockham/gutenberg/releases/tag/untagged-91af46b87ff343e447c9) However, the resulting file will have a somewhat different URL. (The one your reference is GitHub's automatically provided zip of the archive contents, whereas we'll have to attach our own asset to the release.) Is it possible to change that URL wherever it's used to fetch that zip?

Or can whatever mirrors the code from this monorepo into https://github.com/Automattic/jetpack-lazy-images/ run your build step? I don't know.

I think the GH action based approach is cleaner -- it runs the build step when it should be conceptually run, at the time of building a release.

Or can we do release branches that have the files checked in even if they're not checked into master, and live with the composer package not working if someone tries to use "dev-master" or anything else that's not one of those release branches?

🤔 Possibly, but overall, that seems a bit messier...

Note that you're also depending on node_modules/intersection-observer/intersection-observer.js, so that would also have to be checked into the repo if you can't figure out how to do a build step.

Good point, thanks for bringing that up. Ideally, I'd like to avoid checking a node_modules/ dir into GH. We could probably run yarn in the lazy-images directory (to install that dependency) when running the install in the top-level dir or so. (Akin to what lerna does; Calypso should have some precedent for this sort of thing, I'll look into it.)

jeherve · 2020-11-09T15:19:51Z

live with the composer package not working if someone tries to use "dev-master" or anything else that's not one of those release branches?

I don't think that's an option, since we use dev-master ourselves in the Jetpack repo, when building the Jetpack plugin from master or from a branch for testing.

jeherve

This tests well for me. I wonder if it may be worth marking this more than a patch change since we're moving built files around. But absolutely not a blocker, up to you!

The question there is whether the paths or URLs of those JS files would be considered part of the "API" of the package, or just an internal implementation detail. Let's err on the safe side.

anomiex · 2021-06-10T15:53:59Z

The question there is whether the paths or URLs of those JS files would be considered part of the "API" of the package, or just an internal implementation detail. Let's err on the safe side.

.

matticbot · 2021-06-10T16:11:33Z

Caution: This PR has changes that must be merged to WordPress.com
Hello ockham! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D62642-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

github-actions · 2021-06-10T16:32:27Z

Great news! One last step: head over to your WordPress.com diff, D62642-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

anomiex · 2021-06-10T16:33:59Z

r227091-wpcom

ockham added [Status] In Progress [Feature] Lazy Images Build [Package] Lazy Images This package no longer exists in the monorepo. labels Oct 21, 2020

ockham self-assigned this Oct 21, 2020

matticbot added the Touches WP.com Files label Oct 21, 2020

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

ockham force-pushed the update/lazy-images-build-system branch from 3c75041 to b9c3a4e Compare October 22, 2020 12:52

jeherve added this to the 9.1 milestone Oct 26, 2020

jeherve added the [Pri] BLOCKER label Oct 26, 2020

ockham force-pushed the update/lazy-images-build-system branch 2 times, most recently from 39d319e to 3fdb592 Compare November 6, 2020 11:51

ockham marked this pull request as ready for review November 6, 2020 12:52

ockham added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Nov 6, 2020

ockham requested a review from a team November 6, 2020 16:10

brbrr reviewed Nov 6, 2020

View reviewed changes

anomiex previously requested changes Nov 6, 2020

View reviewed changes

anomiex added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 6, 2020

ockham added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 6, 2020

anomiex reviewed Nov 6, 2020

View reviewed changes

anomiex added 2 commits June 10, 2021 09:04

Update pnpm lockfile

d5c0407

Minify intersection-observer.js

0411d07

matticbot added the Touches WP.com Files label Jun 10, 2021

anomiex added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed Touches WP.com Files [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jun 10, 2021

matticbot added the Touches WP.com Files label Jun 10, 2021

anomiex removed the [Status] Blocked / Hold label Jun 10, 2021

jeherve added this to the jetpack/9.9 milestone Jun 10, 2021

jeherve previously approved these changes Jun 10, 2021

View reviewed changes

Minor change, not patch change

49c94d1

The question there is whether the paths or URLs of those JS files would be considered part of the "API" of the package, or just an internal implementation detail. Let's err on the safe side.

anomiex dismissed jeherve’s stale review via 49c94d1 June 10, 2021 15:54

anomiex added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jun 10, 2021

github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Jun 10, 2021

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jun 10, 2021

jeherve approved these changes Jun 10, 2021

View reviewed changes

anomiex merged commit 762408e into master Jun 10, 2021

anomiex deleted the update/lazy-images-build-system branch June 10, 2021 16:31

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jun 10, 2021

anomiex mentioned this pull request Aug 24, 2021

Update webpack globally #20789

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy Images: Use calypso-build to build #17571

Lazy Images: Use calypso-build to build #17571

ockham commented Oct 21, 2020 •

edited

Loading

jetpackbot commented Oct 21, 2020 •

edited

Loading

ockham commented Nov 2, 2020

ockham commented Nov 6, 2020

brbrr Nov 6, 2020

anomiex Nov 6, 2020

ockham Nov 6, 2020

ockham Nov 6, 2020

ockham Nov 6, 2020

ockham Nov 6, 2020

anomiex Nov 6, 2020

anomiex Nov 6, 2020

ockham Nov 6, 2020

anomiex Nov 6, 2020

ockham Nov 6, 2020

anomiex Nov 6, 2020

ockham Nov 6, 2020

ockham commented Nov 6, 2020

ockham commented Nov 6, 2020

anomiex left a comment

anomiex Nov 6, 2020

ockham commented Nov 9, 2020

jeherve commented Nov 9, 2020

jeherve left a comment

anomiex commented Jun 10, 2021

matticbot commented Jun 10, 2021

github-actions bot commented Jun 10, 2021

anomiex commented Jun 10, 2021

Lazy Images: Use calypso-build to build #17571

Lazy Images: Use calypso-build to build #17571

Conversation

ockham commented Oct 21, 2020 • edited Loading

Changes proposed in this Pull Request:

Jetpack product discussion

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

Testing instructions:

Proposed changelog entry for your changes:

Note

jetpackbot commented Oct 21, 2020 • edited Loading

ockham commented Nov 2, 2020

ockham commented Nov 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham commented Nov 6, 2020

ockham commented Nov 6, 2020

anomiex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham commented Nov 9, 2020

jeherve commented Nov 9, 2020

jeherve left a comment

Choose a reason for hiding this comment

anomiex commented Jun 10, 2021

matticbot commented Jun 10, 2021

github-actions bot commented Jun 10, 2021

anomiex commented Jun 10, 2021

ockham commented Oct 21, 2020 •

edited

Loading

jetpackbot commented Oct 21, 2020 •

edited

Loading