Skip to content
This repository was archived by the owner on Oct 4, 2020. It is now read-only.

Wrap FormData effects. #120

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Wrap FormData effects. #120

wants to merge 2 commits into from

Conversation

i-am-tom
Copy link

As the FormData manipulations are effectful, they should live within the
ST monad. This commit exposes the methods for mutating the object behind
the ST wrapper!

As the FormData manipulations are effectful, they should live within the
ST monad. This commit exposes the methods for mutating the object behind
the ST wrapper!
@hdgarrood
Copy link
Contributor

I'm not generally much involved with this repo but since you mentioned it on Slack I thought I might do a quick drive-by review...

Often for an API based on ST you'll want to provide two versions of things: a mutable and immutable version. The mutable version should have a type variable which will match the type variable given to ST, so e.g. FormData for the immutable version and STFormData h for the mutable one: this is necessary to ensure that references to the mutable variable don't escape the scope of the ST computation. Also, with a function like toFormData, the mutation you're doing within the function isn't observable from outside it, so I don't think the consumer needs to know you're using ST internally, and therefore I would recommend not having the ST effect in the type signature.

@hdgarrood
Copy link
Contributor

I think there are some other APIs in this repo which make use of mutation but don't use ST though, so perhaps DOM is enough..?

@garyb
Copy link
Member

garyb commented Aug 29, 2017

I'm afraid this isn't quite right. We'll need a FormDataST type too, to carry the h, and then have a freeze/thaw to convert the mutable FormDataST into FormData, and then only provide the mutation functions for the FormDataST type. Take a look at the STArray and STStrMap stuff for an example of what I'm talking about... if that doesn't make sense still, well, see you on Slack 😄

@garyb
Copy link
Member

garyb commented Aug 29, 2017

Damn it Harry 😆

@hdgarrood
Copy link
Contributor

lmao

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

Successfully merging this pull request may close these issues.

3 participants