search

fable-compiler / fable-promise

Fable bindings for JS promise
http://fable.io/fable-promise/
MIT License
19 stars 7 forks source link

Hide promise-like support being a module ? #22

Closed MangelMaxime closed 3 years ago

MangelMaxime commented 3 years ago

As feared by @alfonsogarciacaro the released of v3 introduced some breaking changes.

One of they is that user need to write return failwithf instead of return! failwithf.

This PR is to explore an idea I had to hide the promise like support being a module. This solution seems to be working for the tests and could bring the best of both world ?

Ping @baronfel @alfonsogarciacaro

alfonsogarciacaro commented 3 years ago

This is great @MangelMaxime, if there's no other impediment this should be the ideal solution. Maybe just changing the name of the module which doesn't convey the purpose to Configurable/Customizable or similar?

Only thing I'm surprised about is this is working with a type extension. I had the impression from #15 that Source had to be implemented in the builder type, and that's why it couldn't be added as an extension directly in Ionide 🤔

MangelMaxime commented 3 years ago

Also as explained in https://github.com/fable-compiler/fable-promise/issues/21#issuecomment-920906947

For me it an "error" (for the lack of a better word) to use return! failwith because failwith doesn't return a Promise.

It was just the F# compiler, being happy to say that this is JS.Promise<...> because failwith returns 'T.

Also this limitation was pointed by @baronfel in his first message but I didn't saw it as the time ^^.

Only thing I'm surprised about is this is working with a type extension. I had the impression from #15 that Source had to be implemented in the builder type, and that's why it couldn't be added as an extension directly in Ionide thinking

I also had this idea in mind not sure where it is coming from.

To me I am still ok with the current v3 release because it is actually forcing the user to use the correct types and not have the type system being abused ^^.

But if we want to go the way of this PR, we should first release a NuGet in beta to see the behavior when used via NuGet. Because, I wonder if this is because we are using ProjectReference for the tests or not. In theory, for Fable it should not change anything because it compiling the source file but perhaps for the IDE their will be a difference?

Maybe just changing the name of the module which doesn't convey the purpose to Configurable/Customizable or similar?

TBH I don't have a really good name in mind so I am ok with your suggestion too.

Also, another suggestion could be PromiseLike because this is a term that I see often in JavaScript ecosystem and represent what we are trying to achieve here.

alfonsogarciacaro commented 3 years ago

Ok, I think I understand now that the purpose of Source was to include the best case for overload resolution in promise CEs, but this seems to work fine if you provide them when extending the CE.

All in all, it looks like we don't actually need to add Source directly to the Promise builder implementation, users can just add it as and extension when needed. Please correct me if I'm wrong.

I will remove Source then to avoid compiler errors in current projects. Then unlist 3.0 and publish 3.1.