chore(gatsby-image): lazy-load refactor

[polarathene]

Description

Light Refactor around the recent changes from adding native lazy-loading support. For some reason most of my review on the PR was ignored(even after requesting feedback after a couple weeks of inactivity).

Best reviewed via individual commits for clarity and each commits message detailing what was done.

Prettier in some cases made some changes less pretty. Related issue on Prettier repo.

---

Questions / Concerns

if (typeof critical === `boolean` && process.env.NODE_ENV !== `production`) {
    logDeprecationNotice(`critical`, `the native "loading" attribute`)

    convertedProps.loading = critical ? `eager` : loading || `lazy`
}

~~Do you really want to only map critical to loading in dev, not production? Or did you only want to raise the deprecation notice in dev? ~~

Is there any reason to use a ternary? Shouldn't it just check if critical is true?

if (critical === true) {
    logDeprecationNotice(`critical`, `the native "loading" attribute`) // Move the env check into this method
    convertedProps.loading = `eager` // If loading wasn't already `lazy` it will be `auto` and the browser can probably make a more informed decision if using eager or lazy is more appropriate.
}

EDIT: Above concern has been addressed. Same as the suggestion above, but no strict equality check since other conditions on the prop just check for truthiness.

---

critical is now advised against being used to have usage replaced with loading attribute instead via this message and docs. However critical is still relevant in the Image constructor and componentDidMount(), this advice may cause issues to be opened due to confusion as the prop is not 1:1 replacement atm.

imgCached will be false in situations that IO isn't used for lazy-load, thus shouldFadeIn won't be disabled, which afaik means we'll probably run into #12254 again?

[polarathene]

Dealing with the failing tests atm. The recent PR for a customizable durationFadeIn somehow passed the tests, I'm confused why, as they should have picked up on the problem with a string value in the test?

That's the only error failing jest from master, I've got a commit for it in this PR to push addressing it.

$ jest ./packages/gatsby-image
 PASS  packages/gatsby-image/src/__tests__/index.js
  <Image />
    ✓ should render fixed size images (44ms)
    ✓ should render fluid images (7ms)
    ✓ should have correct src, title, alt, and crossOrigin attributes (10ms)
    ✓ should have correct placeholder src, title, style and class attributes (5ms)
    ✓ should have a transition-delay of 1sec (9ms)
    ✓ should call onLoad and onError image events (13ms)

  console.error node_modules/prop-types/checkPropTypes.js:19
    Warning: Failed prop type: Invalid prop `durationFadeIn` of type `string` supplied to `Image`, expected `number`.
        in Image (at __tests__/index.js:33)

3 jest snapshot errors due to loading="lazy" now appearing. My PR has changed behaviour from master here, which means I may have made a mistake or corrected the behaviour. Investigating.

UPDATE: Seems warnings can pass the unit tests, I only noticed it due to snapshot mismatch error.

Updated snapshot, it should include the loading attribute regardless since the prop is given a default of lazy. We could omit that and trust the browser to decide when eager or lazy is most appropriate, unless the user provides an explicit choice.

Do you want the test verifying the default lazy value implicitly, or would you like the test to include eager or auto via the loading prop instead with an expect() rule to verify?

[wardpeet]

This is awesome! This looks really good!

test to include eager or auto via the loading prop instead with an expect() rule to verify?

I would do this also check if critical is mapping on eager.

[polarathene]

This should be good to merge, I still need to address the two concerns pointed out at the top after my edit, but I'm a bit busy for the rest of the month.

[wardpeet]

Looks good! thanks for updating. Doing a test run and it will be ready to merge.

[wardpeet]

Looks good! Thanks for cleaning up & thanks for your patience!