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

fix(gatsby-plugin-image): do not show placeholder if image already lo… #28868

Merged
merged 4 commits into from
Jan 12, 2021

Conversation

wardpeet
Copy link
Contributor

@wardpeet wardpeet commented Jan 5, 2021

Description

Move development fix to lazy-hydrate to make sure hydrated.current is not always true to support image caching between routes.

Documentation

Related Issues

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jan 5, 2021
@wardpeet wardpeet added status: needs core review Currently awaiting review from Core team member topic: media Related to gatsby-plugin-image, or general image/media processing topics and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jan 5, 2021
@laurieontech laurieontech marked this pull request as ready for review January 12, 2021 16:05
@laurieontech laurieontech merged commit 12da978 into master Jan 12, 2021
@laurieontech laurieontech deleted the fix/image-caching-route branch January 12, 2021 17:12
@laurenskling
Copy link
Contributor

I'm running 0.6.0-next.1 but you can still see the logo of https://www.ev-auto.nl/ reloading on changing routes

@laurieontech
Copy link
Contributor

Thanks for noting @laurenskling. Do you have a code reproduction? I have to confirm this is in the latest next but I believe it should be, so want to take a closer look.

@laurenskling
Copy link
Contributor

Well, it's kind of hard to isolate this piece of code from my codebase.

this PR is mentioned in the release notes: https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-plugin-image/CHANGELOG.md#060-next1-2021-01-12

in my yarn.lock:

gatsby-plugin-image@^0.6.0-next.1:
  version "0.6.0-query-chunk-stats-base.68"
  resolved "https://registry.yarnpkg.com/gatsby-plugin-image/-/gatsby-plugin-image-0.6.0-query-chunk-stats-base.68.tgz#c59074c28c843803a887e49eaf5c2abb6e810fb3"
  integrity sha512-0iLMGWodv3p9y628n/ALUQbfz0FMcZn5SxXdlL5VJkq1PVz09nTZzqOWkVKIpWkR75pQLmwD53PqHQItBxQmmQ==
  dependencies:
    "@babel/parser" "^7.12.5"
    "@babel/traverse" "^7.12.5"
    babel-jsx-utils "^1.0.1"
    babel-plugin-remove-graphql-queries "2.15.0-next.0"
    camelcase "^5.3.1"
    chokidar "^3.4.3"
    fs-extra "^8.1.0"
    gatsby-core-utils "1.9.0-next.0"
    prop-types "^15.7.2"

@laurenskling
Copy link
Contributor

laurenskling commented Jan 14, 2021

So I dug a little deeper, it's actually from 0.4.0-next.1 to 0.4.0-next.2 the blur up between route changes starts happening. And it's still there in the 0.6.0-next.1

So I guess #28407 is actually not working for me?

@laurieontech
Copy link
Contributor

Just as a test, can you try removing the carat from your version in package.json for gatsby-plugin-image and re-install deps? That version isn't what I expect to see "0.6.0-next.1" is. It may still be an edge case you're hitting, but I want to be sure.

@laurenskling
Copy link
Contributor

Wow I didn't even catch it myself! I'm glad I pasted the lock entry, don't even know why I did! So i actually got 0.6.0-query-chunk-stats-base.68 from ^0.6.0-next.1 and not actually next-1. Removing the carat got me the correct one :) i can't confirm the blur up effect yet, since 'gatsby-plugin-image/compat' seem to have been removed, I need to do some refactoring for some other images first.

new lock:

gatsby-plugin-image@0.6.0-next.1:
  version "0.6.0-next.1"
  resolved "https://registry.yarnpkg.com/gatsby-plugin-image/-/gatsby-plugin-image-0.6.0-next.1.tgz#b80e885abb50f73ad3a0854840a03ce8c26a81dd"
  integrity sha512-Wu+Pc3FA/1glkqJDYd3saCmxEyMN4HHdHPhIScbESeVhtN8fQMROTGHqOFCQqZdykwvc4NhvNIwW9iBHFRntqw==
  dependencies:
    "@babel/parser" "^7.12.5"
    "@babel/traverse" "^7.12.5"
    babel-jsx-utils "^1.0.1"
    babel-plugin-remove-graphql-queries "^2.15.0-next.0"
    camelcase "^5.3.1"
    chokidar "^3.4.3"
    fs-extra "^8.1.0"
    gatsby-core-utils "^1.9.0-next.0"
    prop-types "^15.7.2"
    ```

@laurieontech
Copy link
Contributor

Awesome. If you can, I'd recommend using the codemod to help you refactor. You can even run it on individual files if you like. https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-plugin-image#upgrading-from-the-gatsby-image2

@laurenskling
Copy link
Contributor

Okay, I'm running 0.6.0-next.1 now and between routes the blur up doesn't repeat, but...: when navigating to a different route and clicking the same link again, the image actually goes back to blur and stays there :(

@laurieontech
Copy link
Contributor

So just to clarify, if you go back to the same route the blur up is introduced once? Or it continues to blur up for that page no matter what?

@laurenskling
Copy link
Contributor

I made a branch release: https://feature-image-next--ev-auto.netlify.app/ (be aware that the "header images" of the cars in a showroom are gatsby-image's, not gatsby-plugin-image's, only the logo, the footer logos, and most of the car images are)

So a couple of things are off:

  • First the thing i mentioned before. If you click a menu item and click it again, the logo gets blurred
  • When navigating to Occasions the logo gets blurred
  • going to the detailpage of the car, and sliding a couple of images, images actually get blurred down, not blurred up (i've seen this happening before, on release 0.3 maybe)

@laurieontech
Copy link
Contributor

Ya, seeing the thing with ocassions and the logo. Will dig into it. Thanks for the reproduction! I'll let you know if I get anywhere but it may be challenging without source code.

@laurenskling
Copy link
Contributor

laurenskling commented Jan 14, 2021

I'm actually getting the same behaviour as noted above on 0.4.0-next.1. example: https://60009e331a76d83219f1c799--ev-auto.netlify.app/ So this release "reintroduced" this previous behaviour. Probably, that was fixed by #28407 by the bug that is fixed here 😅

There isn't much special about my code.

some snippets:

a static query containing:

logo {
  name
  url
  width
  height
  imageFile {
    childImageSharp {
      gatsbyImageData(layout: FIXED, width: 80, placeholder: BLURRED)
    }
  }
}

and rendering

import { GatsbyImage } from 'gatsby-plugin-image';
[...]
if (image.imageFile?.childImageSharp?.gatsbyImageData) {
  return (
    <GatsbyImage
      className={className}
      image={logo.imageFile.childImageSharp.gatsbyImageData}
      alt={alternativeText}
      {...props}
    />
  );
}

@laurieontech
Copy link
Contributor

Hey @laurenskling, sorry for the delay. There is a PR in now for a race condition in the images code and I'd love to wait and see if that affects this bug. I'll let you know when it's in next so you can test it out.

@laurenskling
Copy link
Contributor

Yeah sure, drop a comment when it's in; i'll do a new release with that version and we can compare

@laurieontech
Copy link
Contributor

Hey @laurenskling, it should be available in next.

@laurenskling
Copy link
Contributor

Sadly the mentioned behaviour is still present in the latest next (0.7.0-next.1)

https://feature-image-next-7--ev-auto.netlify.app/

@laurieontech
Copy link
Contributor

Thanks for the patience @laurenskling! Fix is forthcoming in #29333

@laurenskling
Copy link
Contributor

Thanks @laurieontech
I've release the fixed to for example: https://feature-image-0-7-1--artzautos.netlify.app/voorraad. You can see when navigating back and forth to this page, the image no longer re-blur-up. Thats awesome.
The thing that does standout for me is that the blur-up now gives a huge blink effect. Like it fades from the background, no longer from the fallback image.
Same here: https://feature-image-0-7-1--ev-auto.netlify.app/voorraad/

@laurenskling
Copy link
Contributor

@laurieontech any ideas about the blink effect? Should I make a seperate issue for it.

The placeholder image gets removed, which makes the opacity transition not working anymore. It will show placeholder -> nothing -> fadein. Its pretty nasty jumping. Example: https://feature-image-1-1-0-next-1--artzautos.netlify.app/voorraad (still present in current version)
The initial load (hard refresh) is okay. If you then refresh you'll get a lot of flashes. It also happens when doing a fresh transition to this page, without cache or anything.

@wardpeet
Copy link
Contributor Author

wardpeet commented Mar 2, 2021

@laurenskling can you disable gatsby-plugin-offline and see if you still have that issue?

@laurenskling
Copy link
Contributor

i've installed gatsby-plugin-remove-serviceworker. A hard refresh on https://feature-image-1-1-0-next-1--artzautos.netlify.app/voorraad don't blink anymore (why would plugin-offline be the case of that?). It is still happening when transitioning to this page tho. Or if you click on any car, you'll see it happen too.

@wardpeet
Copy link
Contributor Author

wardpeet commented Mar 2, 2021

@laurenskling Ok I found the bug it's the if case here:

{!hasLoaded && (
<Placeholder
{...getPlaceholderProps(
placeholder,
isLoaded,
layout,
width,
height,
wrapperBackgroundColor
)}
/>
)}

Could you make a new bug? This can be fixed by using React.memo or make sure we show the placeholder each time.

@laurenskling
Copy link
Contributor

@wardpeet #29911
hope we can fix it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs core review Currently awaiting review from Core team member topic: media Related to gatsby-plugin-image, or general image/media processing topics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants