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

Lazy Images: Improve JS handling #17562

Closed
kraftbj opened this issue Oct 21, 2020 · 3 comments · Fixed by #17571
Closed

Lazy Images: Improve JS handling #17562

kraftbj opened this issue Oct 21, 2020 · 3 comments · Fixed by #17571
Assignees
Labels
[Feature] Lazy Images [Package] Lazy Images This package no longer exists in the monorepo. [Pri] BLOCKER [Type] Bug When a feature is broken and / or not performing as intended

Comments

@kraftbj
Copy link
Contributor

kraftbj commented Oct 21, 2020

Originally reported in #17489 (comment) via @ockham regarding the Lazy Image package JS.

  • We're only bundling code here (through Webpack), not transpiling it (through Babel). This means that statements like const end up in built files, as detected by the ES5 validation script.
  • We're dropping our built files (*.min.js) into src/, rather than a separate location.
  • We're adding built files to GH.
  • We're including the Interaction Observer polyfill verbatim.

In order to fix these issue, [he] suggests

  • using calypso-build (which takes care of transpilation, and by default drops files into a separate build/ directory, thus resolving items 1. and 2.)
  • deleting built files from GH, and adding the build/ subdirectory to .gitignore in order to prevent that from happening in the future
  • adding the Interaction Observer npm to package.json (rather than copying it verbatim)
  • Revert Build: Remove lazy-images ES5 validation #17561 to re-enable validation.
@kraftbj kraftbj added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Lazy Images [Package] Lazy Images This package no longer exists in the monorepo. labels Oct 21, 2020
@kraftbj
Copy link
Contributor Author

kraftbj commented Oct 21, 2020

One thing we'd need to figure out is since this is a Composer package, it is likely expected to be consumed by other plugins without the need to incorporate it into a JS build step? (the original push to get this into a package was for a non-Jetpack consumer: #16657 )

In either case, removing it would break BC for consumers, so that needs to be handled in some way.

@jeherve
Copy link
Member

jeherve commented Oct 21, 2020

We're dropping our built files (*.min.js) into src/, rather than a separate location.
We're adding built files to GH.

I think this is fine. We'd want the package to be ready to use by anyone adding the package to their composer.json file.

@jeherve jeherve added [Pri] BLOCKER [Type] Bug When a feature is broken and / or not performing as intended and removed [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Oct 21, 2020
@jeherve jeherve added this to the 9.1 milestone Oct 21, 2020
@jeherve
Copy link
Member

jeherve commented Oct 21, 2020

using calypso-build

We should be able to update webpack.config.packages.js to start transpiling things, that should solve our issue here and ensure that the Lazy Images lib. can be used in old browsers again.

@jeherve jeherve modified the milestones: 9.1, 9.2 Nov 9, 2020
jeherve added a commit that referenced this issue Nov 10, 2020
This should fix the issues discussed in #17562
@anomiex anomiex modified the milestones: 9.2, 9.3 Nov 24, 2020
@jeherve jeherve removed this from the 9.3 milestone Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Lazy Images [Package] Lazy Images This package no longer exists in the monorepo. [Pri] BLOCKER [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants