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

Include optional params in fetch function #27

Open
yawaramin opened this issue Jun 2, 2019 · 4 comments
Open

Include optional params in fetch function #27

yawaramin opened this issue Jun 2, 2019 · 4 comments

Comments

@yawaramin
Copy link
Contributor

I've come to believe that JavaScript's 'options object' function parameter pattern is a poor-man's optional parameter implementation. The fetch function is a perfect example of this. I would like to wrap it in a function that actually accepts optional parameters and converts them into the requestInit type. This would be a backward-compatible change and look like this:

(* Fetch.ml *)
...
external fetchWithInit : string -> requestInit -> response Js.Promise.t = "fetch" [@@bs.val]
let fetch
  ?method_
  ?headers
  ?body
  ?referrer
  ?referrerPolicy
  ?mode
  ?credentials
  ?cache
  ?redirect
  ?integrity
  ?keepalive
  resource =
  fetchWithInit
    resource
    (RequestInit.make
      ?method_
      ?headers
      ?body
      ?referrer
      ?referrerPolicy
      ?mode
      ?credentials
      ?cache
      ?redirect
      ?integrity
      ?keepalive
      ())

Now callers will be able to use fetch ~method_:Post "http://example.com", instead of fetchWithInit "http://example.com" (RequestInit.make ~method_:Post ()).

@glennsl
Copy link
Contributor

glennsl commented Jun 6, 2019

Hmm, yeah. The original objective for this was to create a set of bindings that were as thin as possible while still being sound, and then have others build libraries on top of that, like re:fetch. But since that hasn't happened and people just use this instead despite the horrible UX, we might as well add some convenience like this.

Now that @bs.string seems to work correctly with polyvariants it might also be possible to actually make these bindings as thin as originally intended, since most of the generated code is just marshaling. Perhaps make one module that compiles to almost nothing, and another that adds a bit of convenience, and weight, like this.

@yawaramin
Copy link
Contributor Author

OK, in that case I'd like to propose making a new Fetch.Raw module that has a 'zero-cost' style, and making the default Fetch module have the convenient wrapper over that. Imho it's more important to provide a friendly UX, especially for newcomers and the (probably large) set of people who don't really care about the added weight. If this is agreeable, I'm happy to work on a PR.

@glennsl
Copy link
Contributor

glennsl commented Jun 7, 2019

Sounds great. Go for it! 😄

@lessp
Copy link

lessp commented Jul 23, 2019

Talking about Refetch which I'd completely missed, perhaps some kind of RFC might be an idea to also try to make a unified API and library which could be used for Native and BS?

I feel as this is a big part, and from lurking it looks like there's some effort going on in a few different places (apart from Refetch):

https://github.com/anuragsoni/fetch/
https://github.com/aantron/repromise
https://github.com/revery-ui/revery-fetch/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants