-
Notifications
You must be signed in to change notification settings - Fork 3
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
[UXIT-1731] Validate Image Src #919
base: main
Are you sure you want to change the base?
Changes from all commits
5575a46
4d1f3e7
43d75d8
9b79ada
3176bb6
667e4c3
15b548b
60c78d2
c5543cd
3012340
ba2300c
2c75747
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,70 @@ | ||||||||||||||||||||||||||||||
'use server' | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
import fs from 'fs/promises' | ||||||||||||||||||||||||||||||
import path from 'path' | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
import Image, { type ImageProps } from 'next/image' | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
import type { ImageObjectFit, StaticImageProps } from '@/types/imageType' | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
import { graphicsData } from '@/data/graphicsData' | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
const { data: fallbackSrc, alt: fallbackAlt } = graphicsData.imageFallback | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
export type SmartImageProps = { | ||||||||||||||||||||||||||||||
src?: string | StaticImageProps['data'] | ||||||||||||||||||||||||||||||
alt?: string | ||||||||||||||||||||||||||||||
className?: string | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm it's included in the line 21? Or you're referring to something else? |
||||||||||||||||||||||||||||||
objectFit?: ImageObjectFit | ||||||||||||||||||||||||||||||
padding?: boolean | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we use it in the Card component, same as |
||||||||||||||||||||||||||||||
} & Omit<ImageProps, 'src' | 'alt' | 'className'> | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
export async function SmartImage({ src, alt = '', ...props }: SmartImageProps) { | ||||||||||||||||||||||||||||||
const imageSrc = await getImageSrc(src) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
const imageExists = imageSrc !== fallbackSrc | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||||
<Image src={imageSrc} alt={imageExists ? alt : fallbackAlt} {...props} /> | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
async function getImageSrc(src: SmartImageProps['src']) { | ||||||||||||||||||||||||||||||
if (!src) { | ||||||||||||||||||||||||||||||
return fallbackSrc | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
const isAssetImage = typeof src === 'string' && src.startsWith('/assets') | ||||||||||||||||||||||||||||||
const isRemoteImage = | ||||||||||||||||||||||||||||||
typeof src === 'string' && (src.startsWith('http') || src.startsWith('//')) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
if (isAssetImage) { | ||||||||||||||||||||||||||||||
const assetExists = await checkAssetImageExists(src) | ||||||||||||||||||||||||||||||
return assetExists ? src : fallbackSrc | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
if (isRemoteImage) { | ||||||||||||||||||||||||||||||
const remoteImageExists = await checkRemoteImageExists(src) | ||||||||||||||||||||||||||||||
return remoteImageExists ? src : fallbackSrc | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
return src | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
async function checkRemoteImageExists(url: string) { | ||||||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||||||
const response = await fetch(url, { method: 'HEAD' }) | ||||||||||||||||||||||||||||||
return response.ok | ||||||||||||||||||||||||||||||
} catch { | ||||||||||||||||||||||||||||||
return false | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
async function checkAssetImageExists(src: string) { | ||||||||||||||||||||||||||||||
const publicPath = path.join(process.cwd(), 'public', src) | ||||||||||||||||||||||||||||||
try { | ||||||||||||||||||||||||||||||
await fs.access(publicPath, fs.constants.F_OK) | ||||||||||||||||||||||||||||||
return true | ||||||||||||||||||||||||||||||
} catch { | ||||||||||||||||||||||||||||||
return false | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
Comment on lines
+62
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This reads a bit better maybe?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes, I like how async await reads, but this made me think we could use try catch to capture errors? what do you think? async function checkAssetImageExists(src: string) {
const publicPath = path.join(process.cwd(), 'public', src);
try {
await fs.access(publicPath, fs.constants.F_OK);
return true;
} catch (error) {
console.error(`Error checking asset image existence at path "${publicPath}":`, error);
return false;
}
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to be explicit to run on the server because
fs
andpath
exclusively run in the node envThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't components
'use server'
by default?I haven't seen this used in components. What does it do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically makes sure it runs server side. For example, in
GovernanceCalendarCard
(use client
component) we importCard
component which usesSmartImage
. if we don't explicitly writeuse server
in theSmartImage
component, it will run on the client, and in that case we get an error because we usefs
which can't run in the browser.