-
Notifications
You must be signed in to change notification settings - Fork 350
Add support for the cf.image
field in fetch()
properties
#351
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
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!) |
38ca3a0
to
76593e5
Compare
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.
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.
76593e5
to
f2e2846
Compare
f2e2846
to
9482480
Compare
9482480
to
ff9c73e
Compare
ff9c73e
to
bf0128c
Compare
bf0128c
to
e824ba7
Compare
Any updates? I really need this … Thanks for doing this @jakubadamw! |
e824ba7
to
890c97c
Compare
@jakubadamw @zebp is there anything outstanding left for this change? I'm happy to pick it up and push it through as well if this one's too stale |
890c97c
to
b66a2b0
Compare
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.
Thank you for the update here, would be great to land this.
6fd1953
to
0f366f0
Compare
…riate Rust + bindgen representations; plus some remaining TODOs and a review comment on related code
… and add the rest of the properties.
0f366f0
to
6fb5497
Compare
Thanks for the dedication here! Will go out in the next release in a couple of weeks. |
Thanks so much for the review and helping get this through! 🤝 |
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.