-
Notifications
You must be signed in to change notification settings - Fork 12
feat: image cdn support #232
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
feat: image cdn support #232
Conversation
…1858-add-passthrough-image-optimization-for-netlifyimages
97cf088 to
655159d
Compare
655159d to
3541d01
Compare
…1858-add-passthrough-image-optimization-for-netlifyimages
…1858-add-passthrough-image-optimization-for-netlifyimages
…1858-add-passthrough-image-optimization-for-netlifyimages
| /** | ||
| * Returns Buffer of a generated random noise jpeg image with the specified width and height. | ||
| */ | ||
| export async function generateImage(width: number, height: number): Promise<Buffer> { |
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.
To avoid adding fixture style images to repo, this will generate images on-demand (random noise, so they don't mean anything, but can be used to assert resizing correctness)
In this PR this is used in ~unit tests for ImageHandler AND e2e for vite plugin
Additionally minor adjustments to Fixture to allow return of this image to feed into Fixture.withFile builder
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
| const imageBuffer = image.data as Buffer |
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.
TS type for the lib that generates random images is ... not working (image is treated as error type) :/
| | Image CDN | ❌ No | | ||
| | Image CDN | ✅ Yes | | ||
|
|
||
| > Note: Missing features will be added incrementally. This module is **not** intended to be a full replacement for the |
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.
First sentence of note right now doesn't make much sense because feature support matrix with this PR is "everything is supported".
Given our previous chats I was thinking about Extensions item and marking as not supported in which case Note itself might not need any changes
| } | ||
|
|
||
| export type ResponseType = 'edge-function' | 'function' | 'redirect' | 'static' | ||
| export type ResponseType = 'edge-function' | 'function' | 'image' | 'redirect' | 'static' |
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.
Should this be considered a breaking change? There is new possible response type for public handleAndIntrospect(NodeRequest) methods
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.
Not sure that's worth a major bump, but happy to defer to you.
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.
I don't think we should consider widening a type as a breaking change
| "vitest": "^3.1.4" | ||
| }, | ||
| "dependencies": { | ||
| "ipx": "^3.0.3" |
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.
👯 👯 👯 netlify/cli#6308
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.
This is good shout, but hopefully this would be addressed by sharp bump that is done in ipx@3 - https://github.com/unjs/ipx/releases/tag/v3.0.0 / lovell/sharp#3750 (vs ipx@2 using older sharp installation that is problematic)
…1858-add-passthrough-image-optimization-for-netlifyimages
8a9a46b to
7621a4d
Compare
| const dev = new NetlifyDev({ | ||
| projectRoot: directory, | ||
| edgeFunctions: { | ||
| // disable edge functions to avoid relying on edge functions handling spinning up internal server |
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.
What do you mean here?
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.
This is currently in main:
primitives/packages/dev/src/main.ts
Lines 392 to 406 in bf00043
| // If a custom server has been provided, use it. If not, we must stand up | |
| // a new one, since it's required for communication with edge functions. | |
| if (typeof this.#server === 'string') { | |
| serverAddress = this.#server | |
| } else if (this.#features.edgeFunctions) { | |
| const passthroughServer = new HTTPServer(async (req) => { | |
| const res = await this.handle(req) | |
| return res ?? new Response(null, { status: 404 }) | |
| }) | |
| this.#cleanupJobs.push(() => passthroughServer.stop()) | |
| serverAddress = await passthroughServer.start() | |
| } |
Handling of local images does rely on origin server, so for the case of ~standalone @netlify/dev without ~external origin server I wanted to have a test that doesn't rely on any side-effects other features might have to make sure it's all self-reliant.
Maybe this could be adjusted to just disable ALL features except for images, because disabling only edge-functions specifically might leak too much of current implementation detail (or have 2 cases - one with default and everything enabled to make sure it all works together and one with just images enabled to test that images handling deal with all its requirements and doesn't rely on other features)
| } | ||
|
|
||
| export type ResponseType = 'edge-function' | 'function' | 'redirect' | 'static' | ||
| export type ResponseType = 'edge-function' | 'function' | 'image' | 'redirect' | 'static' |
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.
Not sure that's worth a major bump, but happy to defer to you.
Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>
| /** | ||
| * Helper to create a server handler that responds with a random noise image. | ||
| */ | ||
| export function createImageServerHandler(imageConfigFromURL: (url: URL) => { width: number; height: number } | null) { |
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.
Not a blocker, but I'm curious why you choose to pass a function here as opposed to something like Record<URL, { width: number, height: number }>?
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.
This felt customizable enough to support potential future test setups that respond to /random-image/:width/:height without having to declare all allowed size combinations ahead of time in tests.
But because nothing like that is actually being done and I could get away with alternative you mentioned (or probably anything heh) for the current tests - I'm happy to adjust, if that would be preferred. I don't have strong feelings here one way or the other.
…1858-add-passthrough-image-optimization-for-netlifyimages
…age-optimization-for-netlifyimages
https://linear.app/netlify/issue/FRB-1838/support-image-optimization
Adds Image CDN support. Should have parity with handling we have in CLI:
ipx(there is transformation from our syntax to ipx syntax which was copied and adjusted from CLI)