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

statsByDimensionsSync generates wrong src on first run #146

Open
timonforrer opened this issue Mar 12, 2022 · 13 comments · May be fixed by #148
Open

statsByDimensionsSync generates wrong src on first run #146

timonforrer opened this issue Mar 12, 2022 · 13 comments · May be fixed by #148

Comments

@timonforrer
Copy link

I'm using sanity studio and their portable text with a serializer, to turn the portable text into html.

The serializer is a shortcode, that looks like this:

const { toHTML } = require('@portabletext/to-html');
const generateImage = require('./image');

const blocks = {
  types: {
    imageWithCaption: ({ value: { image } }) => {
      const {
        width,
        height
      } = image.asset.metadata.dimensions;
      const imageHTML = generateImage({
        src: image.asset.url,
        alt: image.alt,
        sizes: "(min-width: 732px) 80ch, 100vw",
        width,
        height
      });
      return imageHTML;
    }
  }
}

module.exports = function renderHTML(input) {
  return toHTML(input, { components: blocks });
}

And the generateImage() function is:

const Image = require('@11ty/eleventy-img');

module.exports = function imageSyncShortcode(props) {

  const {
    src,
    width,
    height,
    alt,
    sizes,
    loading,
    decoding
  } = props;

  const image_attributes = {
    alt,
    sizes,
    loading: loading ?? 'lazy',
    decoding: decoding ?? 'async',
  }

  const options = {
    widths: [320, 400, 700, 1200, 2000],
    formats: ['avif', 'webp', 'jpeg'],
    urlPath: '/images/',
    outputDir: './dist/images/'
  }

  Image(src, options);

  let metadata = Image.statsByDimensionsSync(src, width, height, options);

  return Image.generateHTML(metadata, image_attributes);
}

I'm using Image.statsByDimensionsSync() so that I don`t have to use async – I tried the async way before, but that didn't work in conjunction with the sanity serializer.

I found that Image.statsByDimensionsSync() generates a different src on the first run. So when I just build the site, the generated html for the image tries to load a file that doesn't exist. When I use eleventy serve and let eleventy rebuild the site once, it works fine.

I assume that eleventy generates the filename based on a different value on the first run than on subsequent runs.

E.g. on the first run the src is /images/CWhDmUxMUw-320.jpeg, on subsequent runs it's /images/Zzo8VdfzGM-320.jpeg

@timonforrer
Copy link
Author

Alright, I was able to lean on sanity's on-demand image processing to generate the HTML for a responsive image in a synchronous fashion:

const client = require('../sanity.js'); // fully configured sanity client, see https://www.sanity.io/docs/js-client
const imageUrlBuilder = require('@sanity/image-url');

const builder = imageUrlBuilder(client);

module.exports = function sanityImage({ source, alt, aspect_ratio, sizes = '100w', loading = 'lazy', decoding = 'async' }) {

  if (alt === undefined) throw new Error('Alt has to be specified');

  // get essential data from asset
  const {
    metadata: {
      dimensions: {
        width: _w,
        aspectRatio: _aspect
      }
    }
  } = source.asset;

  // define widths for output
  let widths = [320, 400, 700, 1200, 2000];

  // filter out widths that are smaller than the actual image
  widths = widths.filter(w => w <= _w);

  // specify output formats and corresponding mimetype
  const formats = {
    webp:'image/webp',
    jpg: 'image/jpeg'
  };

  // generate sources for each format and width
  const sources = Object.keys(formats).map(f => {
    const _sources = widths.map(w => {
      let url, h;
      if (aspect_ratio !== undefined) {
        // custom aspect ratio defined --> crop image
        const [aspect_w, aspect_h] = aspect_ratio.split('x');
        const dec_aspect_ratio = aspect_w / aspect_h;
        h = Math.round(w / dec_aspect_ratio);
        url = builder.image(source).width(w).height(h).format(f).url();
      } else {
        // no custom aspect ratio, just specify width
        h = Math.round(w / _aspect)
        url = builder.image(source).width(w).format(f).url();
      }
      return {
        url,
        w,
        h
      }
    });

    return {
      format: f,
      mimetype: formats[f],
      srcsets: _sources.map(_s => `${_s.url} ${_s.w}w`).join(', '),
      srcs: _sources
    }
  });

  // get last item of `sources` (jpeg) and select first item of `srcs`, which is the smallest size
  const img = sources[sources.length - 1].srcs[0];

  // generate HTML
  const html = `
  <picture>
    ${sources.map(_s => {
      return `
        <source
          type="${_s.mimetype}"
          srcset="${_s.srcsets}"
          sizes="${sizes}"
        />
      `
    }).join('')}
    <img
      alt="${alt}"
      loading="${loading}"
      decoding="${decoding}"
      src="${img.url}"
      width="${img.w}"
      height="${img.h}"
    />
  </picture>`;

  return html;
}

@zeroby0
Copy link
Contributor

zeroby0 commented Mar 15, 2022

E.g. on the first run the src is /images/CWhDmUxMUw-320.jpeg, on subsequent runs it's /images/Zzo8VdfzGM-320.jpeg

Is the image remote?

The filename should be generated by this function:

getHash() {

It reads the contents of the file, and hashes them with SHA256. Usually.
But if it's a remote image, it hashes the URL, and checks if the image in cache is valid.

hash.update(`ValidCache:${this.assetCache.isCacheValid(this.cacheOptions.duration)}`);

That's a place where the hash could change.


Can you see if this bug is reproduceable? If you could make a repository that reproduces this bug, I'll debug it and possibly draft a pull request to fix it.

@timonforrer
Copy link
Author

@zeroby0 Yes, the image is remote.

The bug should be reproducible – I'll share some code with in the next few days, if you wan't to debug it 🙌🏼

@timonforrer
Copy link
Author

timonforrer commented Apr 5, 2022

@zeroby0 I finally took some time trying to make a repo to reproduce the bug. Appearently I was using v1.0.0 of the img package. It works with v2.0.0.

https://github.com/timonforrer/11ty-statsByDimensionsSync/

I'll close this issue

@timonforrer
Copy link
Author

Hi again, @zeroby0

I was a bit hasty.. Bumping the version to v2.0.0 on the repository, that was originally affected by the bug, didn't resolve the issue.

I added a layout and generate the pages using the pagination feature on the reproduction repo. Now the behaviour is as originally described – the img src is wrong on the first build.

https://github.com/timonforrer/11ty-statsByDimensionsSync/

I would be very glad, if you could have a look at it. Thank you!

@timonforrer timonforrer reopened this Apr 14, 2022
@zeroby0
Copy link
Contributor

zeroby0 commented Apr 17, 2022

Heyo @timonforrer

Don't worry, it happens :)

Your repo was very useful in tracing the bug, and it's from this line:

hash.update(`ValidCache:${this.assetCache.isCacheValid(this.cacheOptions.duration)}`);

There is a race condition. If this.assetCache.isCacheValid() is called before the image is in cache, it returns false.

When running for the first time, this.assetCache.isCacheValid() is called before the image could be downloaded and put into the cache. So the hash is updated with ValidCache:false

In the second run, the url is already in the cache, and this.assetCache.isCacheValid() returns true, and the hash is updated with ValidCache:true.

This makes the hash change and return a different src the first time.

zeroby0 added a commit to zeroby0/eleventy-img that referenced this issue Apr 17, 2022
@zeroby0 zeroby0 linked a pull request Apr 17, 2022 that will close this issue
@timonforrer
Copy link
Author

@zeroby0 thank you for looking into it and creating the PR!

@zeroby0
Copy link
Contributor

zeroby0 commented Apr 20, 2022

Thanks for reporting the bug @timonforrer 😄

@Olivier-Rasquin
Copy link

Hello @zeroby0 , I have the exact same issue described by @timonforrer .
Any idea when/if your PR will be validated soon?

Thanks

@zeroby0
Copy link
Contributor

zeroby0 commented Aug 9, 2022

@Olivier-Rasquin I don't know either, I'm eager for it to be reviewed and merged!

I should find a way to gently pester Zach about it XD. Edit: Heard he's on a holiday on Discord. Take a nice break Zach! Let's review it later!

@Olivier-Rasquin meanwhile, you can use

npm r eleventy-img
npm i zeroby0/eleventy-img#issue-146 --save-dev

to temporarily replace eleventy-img with the pull request version.

Update

You should probably trigger builds twice instead of using this fork. I'm not sure that completely solves all the cache bugs either.

Please see the discussion in the accompanies PR for more details.

@Olivier-Rasquin
Copy link

Thanks @zeroby0 ! We've just decided that we will wait for the official "update", and in the meantime we are triggering two successive builds.

Let's hope it can be reviewed fairly soon!

@zeroby0
Copy link
Contributor

zeroby0 commented Aug 29, 2022

I hope so too! Good luck!

@zeroby0
Copy link
Contributor

zeroby0 commented Sep 18, 2022

Hey @zachleat !

Hope you had a great holiday!

When you have the time, can you please take a look at this issue and the fix for it in #148? There's a race condition when fetching remote images causing hash to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants