Make use of purescript-web-promise

[garyb]

It'd be nice if we could switch over to using this so that we have a consistent type for Promises. web-promise isn't one of the heavier web libraries either, adding it to this didn't bring anything else in new transitively.

This is a breaking change, even aside from switching the type of Promise, as now we deal with a Promise.Rejection rather than Foreign in the error coercion function.

[nwolverson]

How does this relate to #29 ?

That PR is just awaiting some decisions on breaking changes

[garyb]

Hah 🤦‍♂️

I'd argue that the implementation here is less surprising/more correct due to the use of thenOrCatch (which didn't exist when the other PR was opened, I added it to web-promise so this could be correctly handled).

In the other PR, if the Aff fails when handling a resolved promise, the error will be passed back into itself. In this one, the Aff will only handle the resolution or rejection of the promise being converted. I don't remember the exact behaviour, but I think things might actually break with the other implementation as I'm pretty sure re-entry is not allowed with Aff.

I also chose to expose Rejection to the error coercion function rather than keeping Foreign, as it makes it easier for the consumer to only fall back when Rejection.toError fails. I guess it could be taken a step further though and actually only call the error coerce after Rejection.toError fails?

I agree with moving away from the Control.Promise namespace. I'd suggest Effect.Aff.Promise rather than Web.Promise.Aff though because of the ordering of the aff and promise components in the name of this library.

[nwolverson]

Paging @YourFin

[YourFin]

Sorry for the delay, my github notifications are crap ($megacorp employer uses something else). Feel free to pester me on discord if I'm not being responsive here; you should be more likely to get a response.

As far as general comments go, I think it might be worth merging our PRs to some extent so you can pick up the extra dependency upgrades in mine (i.e. the spago stuff + updated package.json). That can be done separately though, and otherwise I prefer your PR.

due to the use of thenOrCatch (which didn't exist when the other PR was opened, I added it to web-promise so this could be correctly handled).

🥳 I was looking for this in web-promise when writing my PR, but I had already shaved enough yaks on my way here. Glad to see it!

In the other PR, if the Aff fails when handling a resolved promise, the error will be passed back into itself. In this one, the Aff will only handle the resolution or rejection of the promise being converted. I don't remember the exact behaviour, but I think things might actually break with the other implementation as I'm pretty sure re-entry is not allowed with Aff.

I'm elated to see someone with more expertise taking a look at this. Can we please add a test case to make sure nobody like me comes through and breaks this behavior in the future?

I agree with moving away from the Control.Promise namespace. I'd suggest Effect.Aff.Promise rather than Web.Promise.Aff though because of the ordering of the aff and promise components in the name of this library.

No strong preference on my part here. Including "Web.Promise" in the module name might soothe compatibility fears as a weary user, but that feels like quibbling for my ego's sake more than anything.

[YourFin]

Why not forall a b. Promise.Flatten a b ⇒ Aff a -> Effect (Promise b)?

[sigma-andex]

@i-am-the-slime and I were discussing this topic because we are working on an isomorphic fetch library and just stumbled upon the issue that purescript-web-promise exists in purescript-web and that purescript-aff-promise does not depend on it. Since promises also exist on node, we would propose to move or fork purescript-web-promise to purescript-contrib/purescript-js-promise and make purescript-aff-promise depend on that one then.

[garyb]

I agree with @sigma-andex's proposal - added my 2p on the linked issue there.

@YourFin - my turn to apologise for the delay! All good points, will merge/base off your commit to get the dependencies, etc. / add a couple of tests / relax the Flatten constraint params if it'll let me. Can figure out the naming thing once we deal with main promise library. 🙂

[YourFin]

One other minor thing @garyb - after the included commit it looks like there will be a mix of fancy unicode arrows and the banal ascii ones in the code - could you be sure to update the final version for consistency?

[garyb]

Yes, absolutely. I need to set something up that tells me off for using fancy arrows outside of my/work projects 😄 - I'll add purs-tidy here as that can fix/enforce it.

[garyb]

Finally updated this again!

1. npm, spago and bower dependencies all up to date.
2. Added purs-tidy to check for rogue unicode, etc.
3. Flatten a a has been relaxed.
4. Renamed to Promise.Aff... it is the wrong way around for the library name, but I think it makes more sense, since this is a feature "for" Promise more than one for Aff. If we really wanted to avoid the mix-up I guess we could do the same process with this as we did with js-promise itself - new repo with copied history under the revised name.

I did try adding a test to check the .then(f, g) vs .then(f).catch(g) implementation, but actually seems it works correctly either way due to Aff's machinery: any failure in the Aff code for f is trapped by Aff, so it's impossible (well... inconceivable 😉) for the failure to land back in the promise and fall through to catch.

[nwolverson]

I'll take a full look at the code later, appreciate if @YourFin could do the same (I think your concern was addressed?).

I'm minded to get this PR merged then look to get this repo moved into contrib, as has been repeatedly suggested (whether that should be a transfer or copy I don't know - but can consider the rename also).

[garyb]

Okay, sounds good! I can create a new repo there with the revised name if we want to do it that way.

[YourFin]

A bit late to the party here, but LGTM!

[nwolverson]

@garyb lets do that, recreating in contrib appears to be the best idea if renaming. Were we thinking promise-aff per the namespace?

[garyb]

Yeah, I think so. Will sort it out (create, push history, initial release) later today 👍

[garyb]

js-promise-aff would be another alternative, to align with the js-promise name.

[nwolverson]

I have no problem with either, probably js-promise-aff is clearer

[garyb]

Ok, we have the new repo https://github.com/purescript-contrib/purescript-js-promise-aff and I've pushed the history of this one into that now.

Going to do a few changes so it's configured like the rest of the contrib libraries, then will make a release and get it published.