- Issue created by @balintbrews
- Merge request !1184#3532044: Image component for code components → (Merged) created by Unnamed author
- 🇺🇸United States effulgentsia
This is great! I wonder if the following would be an improvement or not...
What if instead of a wrapper component,
Image from "@/lib/Image"
, people usednext-image-standalone
directly, but we gave them a utility function for getting theloader
function to pass in? For example:import Image from "next-image-standalone"; import {useImageLoader} from "@/lib/utils"; const Cover = ({ image }) => { const loader = useImageLoader(image.srcSetCandidateTemplate); return ( <div> <Image src={image.src} alt={image.alt} width={image.width} height={image.height} loader={loader} /> </div> ); }; export default Cover;
Would the above be more idiomatic in that it would be clearer that people are using essentially the stock Nextjs image component rather than "who knows what else
@/lib/Image
does"? Or is the@/lib/Image
wrapper idea better because it gives us room to add other customizations, like adding some defaults to theconfig
prop? - 🇳🇱Netherlands balintbrews Amsterdam, NL
#4: That's a great idea! It's in line with how we chose to provide
@drupal-api-client/json-api-client
andSWR
instead of a custom wrapper. More idiomatic, as you said. For both we add minimal customization and convenience, but we still keep their original names in the module specifiers. So I think we can do the same if we choose to providenext-image-standalone
directly, as in, we would still have the room to add customizations, such as defaults to theconfig
prop, as you point out.The only potential downside is that a
loader
function is optional withnext/image
: it optimizes images using its own backend by default. I wonder if it could be misleading that we provide something callednext-image-standalone
, which may make it sound like aloader
function is optional, just as it is withnext/image
. Whereas it is required fornext-image-standalone
, which is written in its README.So the way I see it, we need to choose how we would like to communicate what is it that we offer:
- "Here is a component that's almost identical to
next/image
, but you must provide aloader
function. For that we also got you covered, here is a loader function that works with our backend." - "Here is a component you can use to output images. All you need is to pass these simple props. For more control, the component also accepts most props
next/image
does."
- "Here is a component that's almost identical to
- 🇺🇸United States effulgentsia
I prefer #5.1 over #5.2 because it's more similar to shadcn's open code approach. It makes the use of
next/image
(or thenext-image-standalone
version of it which is just a way to getnext/image
outside of a Next.js context) ultimately a decision made by the component author (even if we nudge that decision by providing an example), whereas to me it feels like #5.2 would imply that XB is making that decision more heavy-handedly. - 🇺🇸United States effulgentsia
I wonder if it could be misleading that we provide something called next-image-standalone, which may make it sound like a loader function is optional, just as it is with next/image
Is it reasonable for someone to expect "standalone" (by which is meant: usable outside of Next.js) to have a way of working without providing a loader function?
- 🇳🇱Netherlands balintbrews Amsterdam, NL
#6: Love that, let's go with #5.1. It should be straightforward to adjust the MR to that direction.
#7: I think it depends on how much they understand about what
next/image
does. What's nice aboutnext/image
is that it's not very necessary to fully understand, it mostly just works. Either way, I'm more than okay to let this thought go, and I'm happy with #5.1, a.k.a. #4. - 🇫🇮Finland lauriii Finland
Next.js allows configuring the default loader. I believe this is the most common way of configuring this – it doesn't make sense configuring this per instance unless there's something about a specific instance why it needs to use a different loader.
I think the ideal experience would we that we have a default loader which just works within the Drupal environment. There could be additional loaders for example to use CDN from a DAM to load an image, which would then potentially have to be loaded from within a component.
I don't think this goes against the open code approach because this is already an API that the upstream provides, which I also believe is the primary way of configuring this for Next.js applications.
- 🇺🇸United States effulgentsia
it doesn't make sense configuring this per instance
But where to configure it then? There's no equivalent to
next.config.js
for code components unless we introduce some new convention for code component global JS config. We have a global CSS tab, so maybe adding a global JS tab makes sense?I think the ideal experience would we that we have a default loader which just works within the Drupal environment.
We could do this per #5's answer about adding defaults to
config
. However, the default loader would still require a per-instance prop (srcSetCandidateTemplate
) set that isn't part of the next/image set of props. So #4 could be simplified to:import Image from "next-image-standalone"; const Cover = ({ image }) => { const {src, alt, width, height, srcSetCandidateTemplate} = image; return ( <div> <Image {...{src, alt, width, height, srcSetCandidateTemplate}}/> </div> ); }; export default Cover;
However, I don't think
<Image {...{src, alt, width, height, srcSetCandidateTemplate}}/>
is any friendlier than<Image {...{src, alt, width, height, loader}}/>
. - 🇺🇸United States effulgentsia
Some minor DX improvements we can make to #10...
srcSetCandidateTemplate
is a mouthful. Perhaps this could just besrcTemplate
.- Although in
next/image
, the loader prop is always a function, perhapsnext-image-standalone
could allow it to be either a function or a string, and if it's a string, convert it to a function that replaces{src}
,{width}
, and{quality}
in the string.
Doing both of the above would mean the #10 code would become:
import Image from "next-image-standalone"; const Cover = ({ image }) => { const {src, alt, width, height, srcTemplate} = image; return ( <div> <Image {...{src, alt, width, height}} loader={srcTemplate} /> </div> ); }; export default Cover;
- 🇫🇮Finland lauriii Finland
But where to configure it then? There's no equivalent to next.config.js for code components unless we introduce some new convention for code component global JS config. We have a global CSS tab, so maybe adding a global JS tab makes sense?
Maybe we don't have to introduce a configuration at this point so long as we have set the correct default? I would imagine in Drupal this is something that a module may want to change, similar to what the CDN module is doing.
We could do this per #5's answer about adding defaults to config. However, the default loader would still require a per-instance prop (srcSetCandidateTemplate) set that isn't part of the next/image set of props.
Why do we need
srcSetCandidateTemplate
to be passed per instance? Why would I as the user of that component care about it? I would have no idea based on that prop name why it's there and why I'd have to pass it to the function. Tried to look at the code in the MR and in ✨ Add automated image optimization to image component Active but it still wasn't clear what it is. - 🇺🇸United States effulgentsia
@lauriii: I think you'll like 📌 Improve the front-end DX of Active .
- 🇺🇸United States effulgentsia
Tagging as beta blocker for the same reason as #3515646-51: Add automated
generation → .
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Interesting discussion between @balintbrews & @effulgentsia #4 through #8 🤓
#10: → note that our
AssetLibrary
infrastructure already supports this 😊#11.1: → it is named after the docs at https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/srcset..., specifically the "image candidate string" structure, follow-up for it at 📌 Improve the front-end DX of Active .
Based on #13 through #15, I think this issue is actionable now that ✨ Add automated image optimization to image component Active landed (on Friday); the
srcSetCandidateTemplate
naming change is out of scope here; that's for 📌 Improve the front-end DX of Active . AFAICT next steps here is for @balintbrews to respond to #10 and #11. - 🇳🇱Netherlands balintbrews Amsterdam, NL
#10: "We have a global CSS tab, so maybe adding a global JS tab makes sense?" → note that our AssetLibrary infrastructure already supports this 😊
Yes, we already make use of it for the Tailwind build. That would make it more complicated to do a new tab right now, plus what had been discussed in this issue would also mean inventing a new configuration format. 📌 Improve the front-end DX of Active is a fantastic, elegant solution to avoid that.
This issue is indeed actionable, I'll get to it soon.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
That would make it more complicated to do a new tab right now
Do you mean that it'd be better for this issue to wait for another MR (which one?) to add this tab for the Tailwind build? Or do you mean that the Tailwind build is [going to] just "piggybacking" on that infra and is storing JS in the
global
AssetLibrary
, but won't be showing it in the UI? - 🇳🇱Netherlands balintbrews Amsterdam, NL
#18: The Tailwind build is already piggybacking on that infra since 📌 Compile Tailwind CSS globally for code components Active — storing JS in the
global
AssetLibrary
without showing anything on the UI.In any case, nothing else is needed for this issue, the current MR needs some changes based on the discussion, but nothing major.
- 🇳🇱Netherlands balintbrews Amsterdam, NL
I made the changes based on the discussion, and already future proofed the solution for when 📌 Improve the front-end DX of Active lands.
Here is how the current DX looks like:
import Image from "next-image-standalone"; import { useImageLoader } from "@/lib/use-image-loader"; const Cover = ({ image }) => { const { src, alt, width, height, srcSetCandidateTemplate } = image; const loader = useImageLoader(srcSetCandidateTemplate); return ( <Image {...{ src, alt, width, height }} loader={loader} /> ); }; export default Cover;
After 📌 Improve the front-end DX of Active is in, this will be simplified to the following. Everything is already in place for this by the MR in this issue with a few comments pointing at code that we'll be able to remove. (I also extensively tested on top of the current changes in 📌 Improve the front-end DX of Active .)
import Image from "next-image-standalone"; const Cover = ({ image }) => { const { src, alt, width, height } = image; return <Image {...image} />; }; export default Cover;
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
I tested this with the component in #21 against a fresh install with xb_vite running but after running
npm run build
I get the following failure in the console and it doesn't render a preview.
I have run
npm install
as wellWhat am I missing?
[astro-island] Error hydrating /xb/api/v0/auto-saves/js/js_component/cover SyntaxError: The requested module 'http://127.0.0.1:8080/modules/experience_builder/ui/lib/astro-hydration/dist/jsxRuntime.module.js' doesn't provide an export named: 'jsx' hydration.js:1:3364 Uncaught SyntaxError: The requested module 'http://127.0.0.1:8080/modules/experience_builder/ui/lib/astro-hydration/dist/jsxRuntime.module.js' doesn't provide an export named: 'jsx' jsx-runtime-default.js:1:158
- 🇺🇸United States effulgentsia
I didn't test this MR, but the code looks great to me! I'd consider it RTBC once #22 is addressed.
- 🇳🇱Netherlands balintbrews Amsterdam, NL
I'm very much scratching my head about #22, no idea how Lee managed to hit that. Investigating.
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
If you can't reproduce it, it might just be me doing something wrong
- 🇳🇱Netherlands balintbrews Amsterdam, NL
#27: The steps you wrote in #22 are all the correct ones. I'll ask others to try as well.
- First commit to issue fork.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Paired with @balintbrews & @hooroomoo — turns out that we got the hardcoded widths in
\Drupal\experience_builder\Routing\ParametrizedImageStyleConverter::ALLOWED_WIDTHS
andcomponents/image/image.twig
(they must be manually set to be in sync until 📌 [PP-1] Store allowed widths for xb_parametrized_width in third party settings Postponed ) wrong in ✨ Add automated image optimization to image component Active : we listed only the device sizes, we forgot the image sizes.See https://nextjs.org/docs/pages/api-reference/components/image#imagesizes:
imageSizes allows you to specify a list of image widths. These widths are concatenated with the array of device sizes to form the full array of sizes used to generate image srcset.
Whoops!
Trivial fix here though :)
- First commit to issue fork.
-
balintbrews →
committed 5977ce00 on 0.x
Issue #3532044 by balintbrews, hooroomoo, justafish, larowlan,...
-
balintbrews →
committed 5977ce00 on 0.x
- 🇳🇱Netherlands balintbrews Amsterdam, NL
@lauriii, @hooroomoo, and @justafish helped with testing. We couldn't reproduce #22, but we found smaller issues which were fixed. 🚢
Tested changes on branch
0.x
, following scenarios :- Code Component named as “Image Component “
- Add Image prop and name it as “Image“
- Javascript as :
const Image = ({ image }) => { return (<img {...image} width={x} height={x} className="my-8 max-w-full"/>); }; export default Image