search

verlok / vanilla-lazyload

LazyLoad is a lightweight, flexible script that speeds up your website by deferring the loading of your below-the-fold images, backgrounds, videos, iframes and scripts to when they will enter the viewport. Written in plain "vanilla" JavaScript, it leverages IntersectionObserver, supports responsive images and enables native lazy loading.
https://www.andreaverlicchi.eu/vanilla-lazyload/
MIT License
7.65k stars 674 forks source link

v19.0.0 build error with Hugo #609

Closed XhmikosR closed 5 months ago

XhmikosR commented 5 months ago

Describe the bug v19.0.0 build error with Hugo

To Reproduce Steps to reproduce the behavior:

  1. git clone https://github.com/twbs/blog.git -b xmr/deps
  2. npm i && npm start
  3. See the build error
C:\Users\xmr\Desktop\bootstrap\blog>npm start

> bootstrap-blog@1.0.0 start
> npm run serve

> bootstrap-blog@1.0.0 serve
> hugo server --port 4000 --disableFastRender --noHTTPCache --renderToMemory --printPathWarnings --printUnusedTemplates

Watching for changes in C:\Users\xmr\Desktop\bootstrap\blog\{node_modules,package.json,src}
Watching for config changes in C:\Users\xmr\Desktop\bootstrap\blog\hugo.yml
Start building sites …
hugo v0.124.1-db083b05f16c945fec04f745f0ca8640560cf1ec+extended windows/amd64 BuildDate=2024-03-20T11:40:10Z VendorInfo=gohugoio

Built in 1383 ms
Error: error building site: JSBUILD: failed to transform "/assets/js/lazyload.js" (text/javascript): No matching export in "node_modules/vanilla-lazyload/dist/lazyload.min.js" for import "default"

Expected behavior Should not produce errors.

LazyLoad version Please report which version of LazyLoad you're using.

Going back to v18.0.0 works; you can confirm this with git checkout main && npm i && npm start

verlok commented 5 months ago

Hey @XhmikosR, thank you for opening this. Yeah, we've changed the way LazyLoad is exported, I'm sorry that have impacted your build system. You can rollback safely to 18.0.0 for now, as we try and solve this issue. @erikyo would you have time to check this issue, which was caused by #607?

erikyo commented 5 months ago

Turns out that the umd export has to be set as:

exports: "default",

ref. https://rollupjs.org/configuration-options/#output-exports

@verlok can you kindly assign this one to me?

verlok commented 5 months ago

@erikyo assigned

verlok commented 5 months ago

Hey @XhmikosR, I've just released 19.0.1 with the fix from @erikyo that should fix the problem. Could you please try that and let me know? Thanks

verlok commented 5 months ago

Hey @XhmikosR, I’ve just tried cloning and running your repo as you instructed in your first message, and I'm glad to see the issue is solved!

If you’re happy with our support, the documentation and the effort we've been putting on this project in the latest years, I hope you might want to buy us a coffee to show your appreciation. Or sponsor us, if you're a fan.

Open-source software is great for everyone, but it takes passion and time (and coffee!) to grow and evolve.

Thank you for thinking about it. Have a wonderful day!

XhmikosR commented 5 months ago

Confirmed, thanks!

Hey @XhmikosR, I’ve just tried cloning and running your repo as you instructed in your first message, and I'm glad to see the issue is solved!

If you’re happy with our support, the documentation and the effort we've been putting on this project in the latest years, I hope you might want to buy us a coffee to show your appreciation. Or sponsor us, if you're a fan.

Open-source software is great for everyone, but it takes passion and time (and coffee!) to grow and evolve.

Thank you for thinking about it. Have a wonderful day!

I feel you, I'm on the same boat myself, but OSS just doesn't pay enough, unfortunately.

Thanks again!

erikyo commented 5 months ago

Thank you for the report @XhmikosR! 🤗

And take a look at the new ESM version, can shrink a bit the bundle size and, in general, should perform better than the umd module!

XhmikosR commented 5 months ago

@erikyo unfortunately, I couldn't make Hugo which is using esbuild to use the ESM version even with format: esm in scripts.html.

I'll try to experiment more with ESM later.

erikyo commented 5 months ago

I was able to get it working as a esm module using the module path and replacing this line in this way:

import LazyLoad from 'vanilla-lazyload/dist/esm/lazyload'

Anyhow i'm not sure to have set correctly the esbuild here, is that correct?

{{- $esbuildOptions := dict "target" "es2019" "format" "esm" -}}

if yes it seems the esm path isn't picked correctly, maybe due the "browser" field of the package.json. Thanks for the feedback again, I'll talk to @verlok and we'll see what we can do to improve (if possible).

XhmikosR commented 5 months ago

Yeah, that's the correct solution AFAICT, see https://github.com/twbs/blog/tree/xmr/lazyload-esm.

I would expect esbuild to pick up the ESM build automatically when type: module is set or when an .mjs is used but none worked for me.

XhmikosR commented 5 months ago

Note that on the main repo docs, other packages are correctly picked up as ESM.

git clone https://github.com/twbs/bootstrap.git
npm ci
npm run docs-serve

Then check out search.js or stackblitz.js

XhmikosR commented 5 months ago

OK I think I found the cause. Unsure if this is considered a bug in Hugo/esbuild.

If you change your package.json properties to be relative then it works:

  "main": "./dist/lazyload.min.js",
  "module": "./dist/esm/lazyload.js",
  "browser": "./dist/lazyload.min.js",
  "typings": "./typings/lazyload.d.ts",

EDIT: I was testing the wrong branch, it doesn't fix the issue.

erikyo commented 5 months ago

@XhmikosR but you aren't on the wrong path... the problem is exactly that, we have to look at why Hugo is not picking the esm module (perhaps the main file is not called index and I cannot target build/esm/? file extension? ...)

XhmikosR commented 5 months ago

AFAICT the other two packages we use in the main repo don't use index either (only docsearch does). But StackBlitz correctly uses ESM.

Weird issue for sure 🤔

On Sat, Mar 30, 2024, 09:28 Erik Golinelli @.***> wrote:

@XhmikosR https://github.com/XhmikosR but you aren't on the wrong path... the problem is exactly that, we have to look at why Hugo is not picking the esm module (perhaps the main file is not called index and I cannot target build/esm/? file extension? ...)

— Reply to this email directly, view it on GitHub https://github.com/verlok/vanilla-lazyload/issues/609#issuecomment-2027952867, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACVLNOY35YOH64GGMLPI7DY2ZLRHAVCNFSM6AAAAABFOMFOY2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRXHE2TEOBWG4 . You are receiving this because you were mentioned.Message ID: @.***>

erikyo commented 5 months ago

I would expect esbuild to pick up the ESM build automatically when type: module is set or when an .mjs is used but none worked for me.

That is the changes that we reverted due this issue, https://github.com/verlok/vanilla-lazyload/pull/610

it is however true that Node allows you to export it in various ways, but often not all of them are supported by bundlers so we will have to find the right balance without breaking compatibility with the past and who is using this module.

erikyo commented 5 months ago

Apologies, but I observed the "native lazyload" version and became curious to explore its variances with various bundling methods, both with and without the module. In my testing approach, due to the continual data requests from the YouTube iframe, I rely on the initial 'finish' time recorded in my network panel within the developer tools. All tests are conducted against the localhost version, which retrieves videos from YouTube on the following page: http://localhost:4000/videos/

The "native lazyload" image

Using the esm version of vanilla lazyload (and maybe some others changes) image

using the umd version of vanilla lazyload image

It seems that the esm version is a little faster (as promised) but nothing exceptional of course.. 😇

erikyo commented 5 months ago

I tried and it seems related to the "browser" field of the vanilla-lazyload package. Correctly the Hugo bundler is picking the version for the "browser" as suggested in the package.json, so in order to use the vanilla lazy load "natively" as ESM module even for the browser @verlok should update the "browser" field using the esm path. Not sure about the side-effects so this is only a recap of my findings.

If the repo is left as it is nothing bad happens, the module can be forced specifying using the path of the esm version (e.g. import LazyLoad from 'vanilla-lazyload/dist/esm/lazyload') as we did.

Mystery solved, if you can confirm @XhmikosR, just need to go temporarily to update (or remove) the path in the vanilla-lazyload package.json browser field and you will see (without any further change) that the esm module is loaded as we wanted.

@verlok please keep the module in that way for some time until the ESM support also on the node side is 100% ready.

verlok commented 5 months ago

I just checked the documentation for the package.json and found out that the module field is not there anymore (whilst I have one in my package.json), and it seems it got replaced from the main itself, as per:

The main field is a module ID that is the primary entry point to your program. That is, if your package is named foo, and a user installs it, and then does require("foo"), then your main module's exports object will be returned.

The documentation for the browser field says, instead:

If your module is meant to be used client-side the browser field should be used instead of the main field. This is helpful to hint users that it might rely on primitives that aren't available in Node.js modules. (e.g. window)

The problem is this module is only meant to be used in the browser, so I'm wondering why do I need to have both the browser and the main field populated, instead of only the main.

Thoughts?

verlok commented 5 months ago

I also asked Google Gemini, with this prompt

I'm creating a library useful to lazyload images on the web browser, and I wonder how I should value the main field and the browser field of my package.json. Do I need to value both main and browser, or just the main? In the dist folder I have both the bundled, minified version of the script, which is kinda legacy, and the entry point for importing it as an ES module.

And the following was its reply:

In your case, you'll want to define both main and browser fields in your package.json for wider compatibility. Here's a breakdown of their purposes and why you'll need both for your lazy loading library:

main field:

browser field:

By defining both fields, you cater to different usage scenarios:

Here's an example structure for your dist folder and the corresponding package.json fields:

dist/
  lazyload.min.js (bundled and minified version)
  lazyload.esm.js (entry point for ES module import)

package.json
{
  "main": "dist/lazyload.min.js",
  "browser": "dist/lazyload.esm.js",
  "module": "dist/lazyload.esm.js"  // Optional: can be an alias for "browser" for some bundlers
  // Other properties...
}

This way, your library offers flexibility for both Node.js and browser environments with bundlers.

erikyo commented 5 months ago

yep because that fields were "node related" check here: https://nodejs.org/api/packages.html#conditional-exports

the browser field is a "community condition definition" https://nodejs.org/api/packages.html#community-conditions-definitions and were used to override the "conditional exports"

verlok commented 5 months ago

To be honest I can't understand much from the nodejs.org documentation :D :D

verlok commented 5 months ago

But from my understanding of the npm documentation and from Gemini's answer, I would change both module and browser fields to these values

  "module": "dist/esm/lazyload.js",
  "browser": "dist/esm/lazyload.js",

What do you think @XhmikosR, @erikyo?

erikyo commented 5 months ago

About the "browser" field it's a way to recommend to the bundler which version to use, and of those you actually shipping all 3 would be valid for the browser (iife and esm are the best candidates)

Gemini is telling to serve the esm version as default but my suggestion is to avoid that at the moment to avoid sideeffects like the one that happened to @XhmikosR

the times are changing but we are not 100% yet :)

erikyo commented 5 months ago

At present, you have the option to include a note in the readme specifying that if a user intends to utilize the ECMAScript Module (ESM) version for a bundler, they should reference this specific path instead of the conventional one: import LazyLoad from 'vanilla-lazyload/dist/esm/lazyload.js'

verlok commented 5 months ago

Did that here some days ago

erikyo commented 5 months ago

Yes I think then that it is not necessary to specify anything else... if someone really needs to, I think they will find this issue, which also explains the reason for the current situation and exactly how to solve it.

XhmikosR commented 5 months ago

Agreed, better keep things as is for now. Maybe mention the ESM explicit import in README.md but other than that I guess what we have now is a good compromise.