[gatsby-image] Propagate imgStyle prop down to non-js images #22607

SophieAu · 2020-03-27T16:21:46Z

Description

This fixes the imgStyle prop not being propagated down to the noscript fallback image.

Documentation

N/A

Related Issues

Fixes #21799

pieh · 2020-03-31T17:20:57Z

packages/gatsby-image/src/index.js

+const convertToCSSString = styleObject => {
+  if (!styleObject) return ""
+
+  const cssProps = Object.keys(styleObject)
+  const jsToCSSProp = capitalLetter => `-${capitalLetter[0].toLowerCase()}`
+
+  return cssProps.reduce((cssString, propName) => {
+    const propAsCSS = propName.replace(/([A-Z])/g, jsToCSSProp)
+    return cssString + `${propAsCSS}:${styleObject[propName]};`
+  }, "")
+}
+


I wonder if there is any react-dom helper function we could use for it? It's for sure needed when we render react elements to html string, so would be nice to reuse it and not have to reimplement it.

Other option would be to actually use React to construct content of <noscript> instead of string concatenation and then we wouldn't need to make conversion like that ourselves. Then we could just set style to be an object 🤔

Maybe one of those react-dom functions would do:

https://github.com/facebook/react/blob/fe2cb525542443aaf1447c4069354c12659fd186/packages/react-dom/src/shared/CSSPropertyOperations.js#L24

https://github.com/facebook/react/blob/fe2cb525542443aaf1447c4069354c12659fd186/packages/react-dom/src/server/ReactPartialRenderer.js#L220

I don't think those functions are exported from react-dom module. Looks like they are internal?

yeah, I had a look and they don't seem to be exported. I also had a search around the web to see if there was anything else/any libraries that are tested but had no luck.

Unfortunately the noscript tag seems to need a string as child (it calls toString() or similar on its children), so using React doesn't work either. It will just render as [Object object].

Update: I found the ReactDOMServer.renderToString function (https://reactjs.org/docs/react-dom-server.html) that converts a whole React element to a string which seems to work. I'd love some feedback on the approach though.

Note that the current state of the code isn't cleaned up yet since I wanted to get a yes or no on my approach before making everything pretty

the ReactDOMServer is problematic, because "server" part of react-dom will need to load in browser - functionally it probably will work, but it will also "bloat" our bundles.

The other approach could be figuring out why we use dangerouslySetInnerHTML in

gatsby/packages/gatsby-image/src/index.js

Lines 562 to 574 in 2ea2a8e

{this.addNoScript && (

<noscript

dangerouslySetInnerHTML={{

__html: noscriptImg({

alt,

title,

loading,

...image,

imageVariants,

}),

}}

/>

)}

instead of just treating that that regular component - right now we expect string there, and in master we manually construct it and this PR we actually use react to construct markup but there is still weird indirection there. Maybe we don't need to use it? Then we wouldn't need to look into using additional ReactDOMServer indirections?

Unfortunately the noscript tag seems to convert all its children to string (as if it was) using the built-in toString() function, so just passing a component as a child doesn't work since it will just render as [Object object].

From what I could gather this seems to be the (currently) intended API of noscript: facebook/react#15238

Ah! Ok, this make sense right now.

So I think the ReactDOMServer is only sensibly route to take, but what we need to make sure of is that we only include this for SSR (so .html is correctly generated) and that for client side it doesn't (as the <noscript> should have no effect in browser context anyway as you do need JS enabled in the first place to use browser bundle).

So curse of action I can see here could be something like that:
Instead of importing react-dom/server directly we have extra module (say render-to-string.js) that looks like this:

if (process.env.BUILD_STAGE === `build-html` || process.env.BUILD_STAGE === `develop-html`) { const { renderToString } = require(`react-dom/server`) return renderToString } else { module.exports = () => `` }

This borrows from what we already have in gatsby core runtime ( https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/cache-dir/public-page-renderer.js )

Unfortunate reality of it is that we need to use CommonJS (require / module.exports) instead of ES modules (import/export), because ES modules can't handle conditional imports/exports. There are likely other ways to do similar things (by using webpack aliases, but gatsby-image is not gatsby plugin, so it doesn't have ability to modify gatsby's webpack configuration.

With setup like that we should be able to avoid adding react-dom/server to browser bundles.

What do you think about this approach?

Also /cc @wardpeet, @blainekasten - if anyone from the team would have objection about this, it might be you ;) so would love to hear your thoughts as well. Both about my proposition and maybe suggestions for alternatives

@pieh How far away is the move to v3?

This would be a stop gap solution to not use the string workaround(that this PR mostly removes via ReactDOMServer) only until then as we're only doing this until minimum React version can be bumped to >=16.6(Discussed in April 2019).

Is this worth going forward with until v3, or could we skip it?

pvdz

I have some more superficial, code style related, suggestions.

pvdz · 2020-04-20T14:28:42Z

packages/gatsby-image/src/index.js

-  const mediaAttr = media ? `media="${media}" ` : ``
-  const typeAttr = isWebp ? `type='image/webp' ` : ``
-  const sizesAttr = sizes ? `sizes="${sizes}" ` : ``
+  const propsObj = {};

-  return `<source ${typeAttr}${mediaAttr}srcset="${src}" ${sizesAttr}/>`
+  propsObj["media"] = media;
+  if(isWebp) propsObj["type"] ='image/webp'
+  propsObj["srcset"] = isWebp ? srcSetWebp : srcSet
+  propsObj["sizes"] = sizes
+
+  return <source {...propsObj}/>


Is there a special reason why the property names are dynamic strings rather than regular properties? And since you're creating the object, anyways, you may as well use the shorthand notation.

Also, it looks like prettier/linting wasn't properly applied since this codebase uses backticks for most strings. But linting should would fail in that case anyways so you'd know :)

Suggested change

const mediaAttr = media ? `media="${media}" ` : ``

const typeAttr = isWebp ? `type='image/webp' ` : ``

const sizesAttr = sizes ? `sizes="${sizes}" ` : ``

const propsObj = {};

return `<source ${typeAttr}${mediaAttr}srcset="${src}" ${sizesAttr}/>`

propsObj["media"] = media;

if(isWebp) propsObj["type"] ='image/webp'

propsObj["srcset"] = isWebp ? srcSetWebp : srcSet

propsObj["sizes"] = sizes

return <source {...propsObj}/>

const propsObj = {

media,

type: isWebp ? `image/webp` : undefined,

srcset: isWebp ? srcSetWebp : srcSet,

sizes,

};

Also, if it's relevant whether the type property exists, that would still have to be set conditionally afterwards as JS doesn't have conditional property setting syntax for object literals.

pvdz · 2020-04-20T14:29:46Z

packages/gatsby-image/src/index.js

-        (variant.srcSetWebp ? generateNoscriptSource(variant, true) : ``) +
-        generateNoscriptSource(variant)
+      (variant, i) =>
+        (<React.Fragment key={i}>


Why is the key={i} change necessary? Was this to test something? (The word key doesn't appear anywhere else in this PR and I'm not sure if this is just an API thing)

When generating React elements through iteration, a key prop should be added as per official docs. It should however be uniquely identifiable by including some data related to the current iteration beyond the index, as that kind of defeats the purpose(docs explain why).

pvdz · 2020-04-20T14:32:10Z

packages/gatsby-image/src/index.js

-    : ``
-  const loading = props.loading ? `loading="${props.loading}" ` : ``
-  const draggable = props.draggable ? `draggable="${props.draggable}" ` : ``
+  const propsObj = {}


Same comment as above (if you're creating the object then initialize the properties as far as possible, and only use "dynamic property strings" for property names when you have an explicit reason for it)

Better still, this should just be created as an object literal with all of the properties, rather than creating an empty object and adding them one by one

Yeah, I totally agree. This is just a quick and dirty POC that I haven't cleaned up yet since we haven't agreed yet if my approach is the right one.

packages/gatsby-image/src/index.js

SophieAu · 2020-05-12T10:34:38Z

Note that the code in its current state is just a quick-and-dirty POC while we're still discussing if the approach taken is the right one.

ascorbic · 2020-05-12T11:45:10Z

I think we've agreed that your approach is sound. I'm happy to merge this with a bit of polish

SophieAu · 2020-05-13T18:42:27Z

packages/gatsby-image/src/index.js

+  const {
+    src = "",
+    alt = "",
+    sizes,
+    srcSet,
+    title,
+    width,
+    height,
+    crossOrigin,
+    loading,
+    draggable,
+  } = props

+  const imageProps = {
+    src,
+    alt,
+    sizes,
+    srcSet,
+    title,
+    width,
+    height,
+    crossOrigin,
+    loading,
+    draggable,


not sure if this is prettier or this:

Suggested change

const {

src = "",

alt = "",

sizes,

srcSet,

title,

width,

height,

crossOrigin,

loading,

draggable,

} = props

const imageProps = {

src,

alt,

sizes,

srcSet,

title,

width,

height,

crossOrigin,

loading,

draggable,

const imageProps = {

src: props.src || "",

alt: props.alt || "",

sizes: props.sizes,

srcSet: props.srcSet,

title: props.title,

width: props.width,

height: props.height,

crossOrigin: props.crossOrigin,

loading: props.loading,

draggable: props.draggable,

It's a bit better by compressing the two, as destructing only to build up an object again throws the reader off a bit, and as this list has been growing, it may just be better diff wise for changes with the compact version you're suggesting here.

An alternative via a utility method would allow for the best of both worlds I think, if we can justify the method with usage elsewhere:

function withoutExcess(src, excess) { const trimmed = {}; Object.keys(src) .filter(k => !excess[k]) .forEach(k => { trimmed[k] = src[k]; }); return trimmed; }; // pluck the props to use const { src, alt, sizes, srcSet, title, width, height, crossOrigin, loading, draggable, ...excess } = props // Set defaults in new prop object you'll use them from, then spread the props object with excess props removed const imageProps = { src = ``, alt = ``, ...withoutExcess(props, excess); }

I think the method is pretty easy to read and usage is descriptive for the most part? With this approach, we avoid needlessly repeating ourselves, filtering out additional noise.

pvdz · 2020-06-06T13:45:59Z

I ran the formatter over it and lgtm otherwise. Removing myself as reviewer.

polarathene

Drop noscript variant methods

generateNoscriptSource() and generateNoscriptSources() are made redundant by this PR. Use the existing methods when a string response isn't needed instead.

Migrate noscript tag into noscriptImg

The tag and usage of dangerouslySetInnerHTML prop would be better suited grouping in the component, especially with ReactDOMServer being added. A TODO comment for v3 refactor can be added to ensure both workarounds can be removed(see review comment for suggested text).

Make DRY (Maintenance) - Optional

noscriptImg should try to reduce duplicate code that can be shared with Img it's already fallen out of sync with some props it seems beyond imgStyle. This isn't super important for this PR as a follow-up can cover refactoring(I'd be happy to do it after existing PRs for this package are reviewed, a few have accumulated lately).

If Img cannot be wrapped effectively due to some differences, it'd be good to extract common props and the base style that is shared.

This snippet might be useful during a refactor.

polarathene · 2020-06-30T02:41:10Z

packages/gatsby-image/src/index.js

 function generateNoscriptSources(imageVariants) {
-  return imageVariants
-    .map(
-      variant =>
-        (variant.srcSetWebp ? generateNoscriptSource(variant, true) : ``) +
-        generateNoscriptSource(variant)
-    )
-    .join(``)
+  return imageVariants.map((variant, i) => (
+    <>
+      {!!variant.srcSetWebp && generateNoscriptSource(variant, true)}
+      {generateNoscriptSource(variant)}
+    </>
+  ))


As mentioned with the child method called. This would be redundant, use generateImageSources() instead.

Despite that, you're adding i as a map fn param, but you don't use it here. The !!variant should also be unnecessary. None of the logic needs to be modified, just use the non-string generator method instead that we already have.

polarathene · 2020-06-30T03:04:17Z

packages/gatsby-image/src/index.js

 const noscriptImg = props => {
-  // Check if prop exists before adding each attribute to the string output below to prevent
-  // HTML validation issues caused by empty values like width="" and height=""
-  const src = props.src ? `src="${props.src}" ` : `src="" ` // required attribute
-  const sizes = props.sizes ? `sizes="${props.sizes}" ` : ``
-  const srcSet = props.srcSet ? `srcset="${props.srcSet}" ` : ``
-  const title = props.title ? `title="${props.title}" ` : ``
-  const alt = props.alt ? `alt="${props.alt}" ` : `alt="" ` // required attribute
-  const width = props.width ? `width="${props.width}" ` : ``
-  const height = props.height ? `height="${props.height}" ` : ``
-  const crossOrigin = props.crossOrigin
-    ? `crossorigin="${props.crossOrigin}" `
-    : ``
-  const loading = props.loading ? `loading="${props.loading}" ` : ``
-  const draggable = props.draggable ? `draggable="${props.draggable}" ` : ``
+  const {
+    src = ``,
+    alt = ``,
+    sizes,
+    srcSet,
+    title,
+    width,
+    height,
+    crossOrigin,
+    loading,
+    draggable,
+  } = props


If leveraging the (almost) same destructure from Img isn't possible, the two components should share what they can through a prop filter method, and then only handle the extra prop extraction that's unique. Note that this noscript variant appears to have fallen out of sync a little, and some props like ariaHidden(aria-hidden) may not spread onto img correctly? Spreading what is possible is probably a good idea though.

polarathene · 2020-06-30T03:10:18Z

packages/gatsby-image/src/index.js

+    style: {
+      position: `absolute`,
+      top: 0,
+      left: 0,
+      opacity: 1,
+      width: `100%`,
+      height: `100%`,
+      objectFit: `cover`,
+      objectPosition: `center`,
+      ...props.imgStyle,
+    },


Share the base style from Img, if the Img component can be re-used instead, just pass in imgStyle instead of imageStyle(adds conditional opacity/transition styles) to the style prop.

polarathene · 2020-06-30T03:27:03Z

packages/gatsby-image/src/index.js

+  return ReactDOMServer.renderToString(
+    <picture>
+      {sources}
+      <img {...imageProps} />
+    </picture>
+  )


Nice workaround :)

I guess this would work until v3, but maybe add a TODO comment about that, like:

// TODO: In v3, minimum required React will be >=16.6, 'dangerouslySetInnerHTML' and 'ReactDOMServer' workarounds will no longer be necessary and can be dropped

Additionally, it would make sense to move the <noscript /> element and it's dangerouslySetInnerHTML prop into this component and return that after providing it the string? Should isolate/group the issue better that way for the v3 refactor.

polarathene · 2020-06-30T03:32:45Z

packages/gatsby-image/src/index.js

@@ -568,6 +597,7 @@ class Image extends React.Component {
                  alt,
                  title,
                  loading,
+                  imgStyle,


If the fixed/fluid components instead move this noscript tag into it's own component, it could be a wrapper component to Img(or rather the React equivalent picture element also moved out into it's own component, a third component would take in the props to pass to each, along with the two conditional props to control state, generateImageSources() would only need to be called once then).

Not too important, a follow-up PR could do that.

polarathene · 2020-06-30T03:37:11Z

packages/gatsby-image/src/index.js

 function generateNoscriptSource({ srcSet, srcSetWebp, media, sizes }, isWebp) {
-  const src = isWebp ? srcSetWebp : srcSet
-  const mediaAttr = media ? `media="${media}" ` : ``
-  const typeAttr = isWebp ? `type='image/webp' ` : ``
-  const sizesAttr = sizes ? `sizes="${sizes}" ` : ``
+  const props = {
+    media,
+    sizes,
+    srcset: isWebp ? srcSetWebp : srcSet,
+    type: isWebp ? `image/webp` : undefined,
+  }

-  return `<source ${typeAttr}${mediaAttr}srcset="${src}" ${sizesAttr}/>`
+  return <source {...props} />
 }


This change is redundant. Use generateImageSources() instead if string generation is not needed anymore(requires min version of React for the package to be raised to >=16.6, last I knew that change is delayed until the next major version.

wardpeet · 2020-11-23T10:15:56Z

Closing in favor of #27950, it's a total rewrite and fixes this issue

SophieAu added 4 commits March 27, 2020 14:44

Propagate imgStyle to noscript version

529b611

Use the imgStyle prop instead of imageStyle

d84ec46

Propagate props for both image types

f1887ff

Propely build styles from props

43cd6ac

SophieAu requested a review from a team as a code owner March 27, 2020 16:21

pieh reviewed Mar 31, 2020

View reviewed changes

SophieAu added 2 commits April 17, 2020 14:54

Build noscript in React then print to html string

8de2f45

Minor cleanup

8bb5ae1

pvdz suggested changes Apr 20, 2020

View reviewed changes

wardpeet self-assigned this May 6, 2020

chore: format

5b50ef7

gatsbybot and others added 3 commits May 12, 2020 11:46

Merge remote-tracking branch 'upstream/master' into patch-1

60574ee

chore: format

06e1ab9

Cleanup

b5ec0eb

SophieAu commented May 13, 2020

View reviewed changes

Remove 'key' prop

822cd35

danabrit added the topic: media Related to gatsby-plugin-image, or general image/media processing topics label May 29, 2020

SophieAu requested review from pvdz, ascorbic and pieh June 6, 2020 13:18

chore: format

599ee38

pvdz removed their request for review June 6, 2020 13:46

polarathene suggested changes Jun 30, 2020

View reviewed changes

polarathene mentioned this pull request Jul 3, 2020

[Tracking]: gatsby-image PRs waiting on review and merge #25487

Closed

7 tasks

etabard mentioned this pull request Jul 8, 2020

gatsby-image noscript img doesn't honor imgStyle #24617

Closed

wardpeet closed this Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[gatsby-image] Propagate imgStyle prop down to non-js images #22607

[gatsby-image] Propagate imgStyle prop down to non-js images #22607

SophieAu commented Mar 27, 2020

pieh Mar 31, 2020

flogy Apr 6, 2020

vladar Apr 16, 2020 •

edited

Loading

SophieAu Apr 17, 2020 •

edited

Loading

SophieAu Apr 17, 2020 •

edited

Loading

pieh Apr 17, 2020

SophieAu Apr 17, 2020

pieh Apr 17, 2020

polarathene Jun 30, 2020 •

edited

Loading

pvdz left a comment

pvdz Apr 20, 2020

pvdz Apr 20, 2020

polarathene Jun 30, 2020

pvdz Apr 20, 2020

ascorbic May 12, 2020

SophieAu May 12, 2020

SophieAu commented May 12, 2020

ascorbic commented May 12, 2020

SophieAu May 13, 2020

polarathene Jun 30, 2020

pvdz commented Jun 6, 2020

polarathene left a comment •

edited

Loading

polarathene Jun 30, 2020

polarathene Jun 30, 2020

polarathene Jun 30, 2020

polarathene Jun 30, 2020

polarathene Jun 30, 2020

polarathene Jun 30, 2020

wardpeet commented Nov 23, 2020

	{this.addNoScript && (
	<noscript
	dangerouslySetInnerHTML={{
	__html: noscriptImg({
	alt,
	title,
	loading,
	...image,
	imageVariants,
	}),
	}}
	/>
	)}

[gatsby-image] Propagate imgStyle prop down to non-js images #22607

[gatsby-image] Propagate imgStyle prop down to non-js images #22607

Conversation

SophieAu commented Mar 27, 2020

Description

Documentation

Related Issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladar Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

SophieAu Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

SophieAu Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polarathene Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

pvdz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SophieAu commented May 12, 2020

ascorbic commented May 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvdz commented Jun 6, 2020

polarathene left a comment • edited Loading

Choose a reason for hiding this comment

Drop noscript variant methods

Migrate noscript tag into noscriptImg

Make DRY (Maintenance) - Optional

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet commented Nov 23, 2020

vladar Apr 16, 2020 •

edited

Loading

SophieAu Apr 17, 2020 •

edited

Loading

SophieAu Apr 17, 2020 •

edited

Loading

polarathene Jun 30, 2020 •

edited

Loading

polarathene left a comment •

edited

Loading