Image component for code components

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 used next-image-standalone directly, but we gave them a utility function for getting the loader 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 the config prop?

#4: That's a great idea! It's in line with how we chose to provide @drupal-api-client/json-api-client and SWR 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 provide next-image-standalone directly, as in, we would still have the room to add customizations, such as defaults to the config prop, as you point out.

The only potential downside is that a loader function is optional with next/image: it optimizes images using its own backend by default. 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. Whereas it is required for next-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:

1. "Here is a component that's almost identical to next/image, but you must provide a loader function. For that we also got you covered, here is a loader function that works with our backend."
2. "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."

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 the next-image-standalone version of it which is just a way to get next/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.

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?

#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 about next/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.

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.

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}}/>.

Some minor DX improvements we can make to #10...

1. srcSetCandidateTemplate is a mouthful. Perhaps this could just be srcTemplate.
2. Although in next/image, the loader prop is always a function, perhaps next-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;

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.

@lauriii: I think you'll like 📌 Improve the front-end DX of Active .

Proposal on that issue looks great! 👏

Tagging as beta blocker for the same reason as #3515646-51: Add automated generation → .

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.

#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.

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?

#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.

👍

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;

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 well

What 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

Code changes look good to me, just the issue with manual testing per #22

I didn't test this MR, but the code looks great to me! I'd consider it RTBC once #22 is addressed.

I'm very much scratching my head about #22, no idea how Lee managed to hit that. Investigating.

If you can't reproduce it, it might just be me doing something wrong

#27: The steps you wrote in #22 are all the correct ones. I'll ask others to try as well.

Paired with @balintbrews & @hooroomoo — turns out that we got the hardcoded widths in \Drupal\experience_builder\Routing\ParametrizedImageStyleConverter::ALLOWED_WIDTHS and components/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 :)

@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