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

Webapi.FormData.t should not be a record type #84

Closed
yawaramin opened this issue Jan 15, 2022 · 3 comments · Fixed by #87
Closed

Webapi.FormData.t should not be a record type #84

yawaramin opened this issue Jan 15, 2022 · 3 comments · Fixed by #87
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@yawaramin
Copy link
Contributor

It is currently defined as:

type t = private {
  keys: Webapi__Iterator.t<string>,
  values: Webapi__Iterator.t<EntryValue.t>,
  entries: Webapi__Iterator.t<(string, EntryValue.t)>,
}
...
@send external keys: t => Webapi__Iterator.t<string> = "keys"
@send external values: t => Webapi__Iterator.t<EntryValue.t> = "values"
@send external entries: t => Webapi__Iterator.t<(string, EntryValue.t)> = "entries"

Notice that keys(), values(), and entries() are nullary methods, not getters. Exposing record fields like keys: Webapi__Iterator.t<string> is incorrect because those are not the correct types. This type should be reverted back to an abstract type.

Relates to #2

@TheSpyder
Copy link
Owner

This happened in 2ba24b0 when I added the fetch bindings. They were @send before, not @get, I don’t know what I was thinking.

@TheSpyder TheSpyder added bug Something isn't working good first issue Good for newcomers labels Jan 15, 2022
@TheSpyder
Copy link
Owner

In theory the record fields could be defined as functions, but in my experience calling a record function always generates a Curry call at runtime. So I agree the type should revert to abstract.

TheSpyder added a commit that referenced this issue Jan 25, 2022
…e externals are kept for compatibility)"

Fixes #84.

This reverts commit 2ba24b0.
TheSpyder added a commit that referenced this issue Jan 25, 2022
…e externals are kept for compatibility)"

Fixes #84.

This reverts commit 2ba24b0.
@TheSpyder
Copy link
Owner

Released as 0.5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants