Add support for the cf.image field in fetch() properties
#351
Conversation
|
@zebp any chance this could be reviewed? 🙂 Seems like there generally is a lot of outstanding PRs open and ready to be reviewed. I wonder what the best process of getting one's PR reviewed and merged is here? I think it would make sense to perhaps document the process in the README or, otherwise, external contributors may get discouraged from getting involved, seeing their contributions not picked up for review long after submission? (General observation, nothing personal!) |
jakubadamw
force-pushed
the
fetch-add-cf-image-property
branch
from
38ca3a0
to
76593e5
Compare
There was a problem hiding this comment.
Some minor changes needed, but looks like it's on the right track.
And yeah I think you hit the nail on the head about the PRs, there's definitely more that should be done to make sure PRs don't stack up and get reviewed in a more timely manner. I like the suggestion of documenting a process for contribution, but I also will start to be more active in reviewing and dealing with issues. In the future please feel free to ping me directly, my Github inbox is a mess but any direct pings give me a notification on my phone that I do check.
| } | ||
| /// Configuration options for Cloudflare's image resizing feature: | ||
| /// <https://developers.cloudflare.com/images/image-resizing/> | ||
| #[derive(Clone, Default, serde::Serialize)] |
There was a problem hiding this comment.
This should just be Serialize to be consistent with other structs in the module.
| pub enum RequestRedirect { | ||
| Error, | ||
| #[default] | ||
| Follow, | ||
| Manual, | ||
| } | ||
|
|
||
| impl From<RequestRedirect> for &str { |
There was a problem hiding this comment.
I know these Into<&str> impls aren't necessary anymore, but it'd be nice to not introduce any breaking changes however unlikely they might actually be.
|
|
||
| #[derive(Clone, Copy, Serialize)] | ||
| #[serde(rename_all = "kebab-case")] | ||
| pub enum ResizeFormat { |
There was a problem hiding this comment.
There's also jpeg and baseline-jpeg https://developers.cloudflare.com/images/image-resizing/resize-with-workers#format
| /// Configuration options for Cloudflare's image resizing feature: | ||
| /// <https://developers.cloudflare.com/images/image-resizing/> | ||
| #[derive(Clone, Default, serde::Serialize)] | ||
| pub struct ResizeConfig { |
There was a problem hiding this comment.
There's some more properties defined in the TS types that aren't here like draw and it'd be really nice if these fields were documented as it can be a little overwhelming seeing such a big struct.
https://workers-types.pages.dev/#RequestInitCfPropertiesImage should be handy for porting
jakubadamw
force-pushed
the
fetch-add-cf-image-property
branch
from
76593e5
to
f2e2846
Compare
jakubadamw
force-pushed
the
fetch-add-cf-image-property
branch
from
f2e2846
to
9482480
Compare
jakubadamw
force-pushed
the
fetch-add-cf-image-property
branch
from
9482480
to
ff9c73e
Compare
jakubadamw
force-pushed
the
fetch-add-cf-image-property
branch
from
ff9c73e
to
bf0128c
Compare
…riate Rust + bindgen representations; plus some remaining TODOs and a review comment on related code
… and add the rest of the properties.
jakubadamw
force-pushed
the
fetch-add-cf-image-property
branch
from
bf0128c
to
e824ba7
Compare
Per the definition in https://github.com/cloudflare/workerd/blob/4fb4dbd88d30ff293267680d85a31ddef94fd9b6/types/defines/cf.d.ts#L171-L295.
This is based on the existing #233 authored by @seeekr, with some improvements and the completion of the set of properties supported.