Skip to content
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

Request and Response constructors? #63

Closed
tom-sherman opened this issue Dec 20, 2021 · 3 comments · Fixed by #64
Closed

Request and Response constructors? #63

tom-sherman opened this issue Dec 20, 2021 · 3 comments · Fixed by #64
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@tom-sherman
Copy link
Contributor

Are these purposefully omitted or would you accept PRs for them?

I'm currently working on some Remix and was looking to depend on this package for the webapi types.

@TheSpyder
Copy link
Owner

I believe it does have some bindings in this area, we pulled in bs-fetch as Webapi.Fetch:

https://github.com/tinymce/rescript-webapi/blob/f1194750136d1ac7b674ef1f21428f63213e2bbd/src/Webapi/Webapi__Fetch.res#L349-L362

We have #30 logged to update the fetch API in a more modern ReScript style after 1.0. For now we're trying to be as compatible as possible to help existing bs-webapi users to migrate.

If this doesn't meet your needs then yes we will accept PRs. You might be able to find some discussion in either the bs-fetch or bs-webapi repos that can talk to why they may have been omitted... but we are re-evaluating all decisions with this update anyway.

@tom-sherman
Copy link
Contributor Author

tom-sherman commented Dec 20, 2021

@TheSpyder Thanks for the context! I seems to be me that while there is a way of creating a plane RequestInit object, there is no way of passing that to Request.make. There is also no way to construct a ResponseInit object or a Response instance.

So, I see two changes that could be made:

  • Allow passing a RequestInit.t to Request.make
  • Add a corresponding ResponseInit.make function and allow passing it to a Response.make function. ((Webapi.Fetch.BodyInit.t, ResponseInit.t) => Webapi.Fetch.Response.t)

Here's what I'm doing currently to work around the lack of Response constructor: https://github.com/tom-sherman/remix-rescript-example/blob/bfed82c4a26d1121b5198d7cd3e4872d4399cc03/app/entry.server.res#L1-L11

@TheSpyder
Copy link
Owner

It's already possible to pass a RequestInit.t to Request.make:
https://github.com/tinymce/rescript-webapi/blob/18d9d553201d575fa8e194d3370298b556b28747/src/Webapi/Webapi__Fetch.res#L386-L389

I'd be happy to accept a PR that adds matching Response.make and ResponseInit.make methods in a similar style. That's a non-breaking change. Hopefully we can find ways to make this better in a later shake-up of the fetch API bindings!

@TheSpyder TheSpyder added enhancement New feature or request help wanted Extra attention is needed labels Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants