Zaid-Ajaj / Fable.Remoting

Type-safe communication layer (RPC-style) for F# featuring Fable and .NET Apps

https://zaid-ajaj.github.io/Fable.Remoting/

MIT License

272 stars 54 forks source link

Consolidate `send` and `sendAndReadBinary` implementations into one method #239

Closed schauerte closed 3 years ago

schauerte commented 3 years ago

We came across this somewhat duplicated code (of send and sendAndReadAsBinary) while trying to add cancellation support to Fable.Remoting. So to not end up changing both of these methods for this, we would kindly suggest using only one implementation for handling the XMLHttpRequest.

I hope this request is considered meaningful and our changes are a feasible way to achieve this - otherwise please let us know.

Zaid-Ajaj commented 3 years ago

Hi @schauerte to be honest with you, I don't mind code duplication especially in cases where the code is pretty simple. I am wondering though, is this PR part of #238 (add cancellation support) and if so, why not make into the same PR?

schauerte commented 3 years ago

238 is exactly, why we came across this - at first we only enhanced one of these methods (overlooked the other one) and wondered why our changes do not work...

Of course duplication (of simple code) is fine - we just differ in what looks simple to us.

I wanted to clarify if you are OK with the consolidation or prefer the separated approach (for any reason) before creating a pull request for the actual cancellation change - but @do-wa and I talked past each other and he went along creating #238. We are happy to create one PR for both . just let us know if we should combine them or simply abandon this one.

Zaid-Ajaj commented 3 years ago

let us know if we should combine them or simply abandon this one.

Would be great if you can combine into one PR please 🙏 thanks!

schauerte commented 3 years ago

moved to #238: https://github.com/Zaid-Ajaj/Fable.Remoting/pull/238/commits/d498bf75c0825f57cf34c38ef1aa486207cf3b49 Thank you very much and sorry for the confusion.