Conversation
Does not fully implement RequestInit, only what seemed useful. Closes: gleam-lang#4
lpil
left a comment
lpil
left a comment
There was a problem hiding this comment.
Hello! Thank you for this. I'm sure people will find it very useful.
We never use _with suffixes in Gleam core, so we'll need to adjust this design slightly. Could you adopt the same pattern that the gleam_httpc library uses please: https://hexdocs.pm/gleam_httpc/gleam/httpc.html
I've left some notes inline with more details.
test/gleam_fetch_test.gleam
Outdated
| |> fetch_options.set_credentials(fetch_options.CredentialsOmit) | ||
| |> fetch_options.set_keepalive(True) | ||
| |> fetch_options.set_priority(fetch_options.High) | ||
| |> fetch_options.set_redirect(fetch_options.Follow) |
There was a problem hiding this comment.
What does this test verify? Doesn't seem like there's assertions for any of these?
There was a problem hiding this comment.
I've expanded on this one and tried to write all of the tests, sadly node-fetch polyfill doesn't implement most of this stuff, so it's not testable. Since node is running V8, it does however error out on wrong values, therefore simply assigning values and checking whether requests succeed is imo good enough.
src/gleam/fetch/fetch_options.gleam
Outdated
| @@ -0,0 +1,184 @@ | |||
| import gleam/dynamic.{type Dynamic} | |||
|
|
|||
| /// Gleam equivalent of JavaScript [`RequestInit`](https://developer.mozilla.org/docs/Web/API/RequestInit). | |||
There was a problem hiding this comment.
Wrap doc comments at 80 lines like regular Gleam code, and included documentation for each option rather solely linking to an external resource 🙏
There was a problem hiding this comment.
Tried to include "something" it's still mostly Mozilla docs shortened.
duzda
force-pushed
the
fetch-options
branch
from
4e5e471 to
dc8783d
Compare
|
Hi @lpil! I've tried to implement most of your comments. Moved some stuff around, expanded more on the tests and docs. Regarding docs, I'm kinda lost what more to include. Definitely expecting few more changes regarding naming. Also will squash once ready to merge. |
2 participants
Does not fully implement RequestInit, only what seemed useful.
Closes: #4
Deprecates: #5