apollo-datasource-http icon indicating copy to clipboard operation
apollo-datasource-http copied to clipboard

Published 20 hours ago • verified publisher icon StarpTech

Memoize promises instead of results to avoid concurrency problems

Open llc1123 opened this issue 2 years ago • 16 comments

Fixes #34

llc1123 avatar Apr 17 '22 12:04 llc1123

Anything blocking this PR? @StarpTech

llc1123 avatar May 05 '22 15:05 llc1123

Time to review :smiling_face_with_tear: I'll try to review it in the next days.

StarpTech avatar May 05 '22 16:05 StarpTech

I would love to use this! Any updates on blocking review @StarpTech ?

tdipadova3rd avatar Jun 21 '22 19:06 tdipadova3rd

@llc1123 please fix the conflict. I'm going to review it in the next days.

StarpTech avatar Jun 21 '22 22:06 StarpTech

@StarpTech fixed.

llc1123 avatar Jun 24 '22 09:06 llc1123

Really looking forward to this PR - the memoization shortcoming that this PR addresses has been a blocker for us.

bradleymorris avatar Jun 30 '22 15:06 bradleymorris

Did you run the benchmarks?

StarpTech avatar Jun 30 '22 17:06 StarpTech

Updated. @StarpTech

llc1123 avatar Jul 07 '22 07:07 llc1123

Any updates @StarpTech ? Would love to use these changes.

tdipadova3rd avatar Jul 12 '22 00:07 tdipadova3rd

Any updates @StarpTech ? Would love to use these changes.

same , waiting on this update 👍

0xTea avatar Jul 12 '22 07:07 0xTea

@StarpTech Any updates? Or should I fork for my use case?

tdipadova3rd avatar Aug 03 '22 12:08 tdipadova3rd

@tdipadova3rd sorry for the delay. I'll review and merge this PR this week :crossed_fingers:

StarpTech avatar Aug 16 '22 20:08 StarpTech

@llc1123 I appreciate the refactoring, but you touched way too many places which aren't related to the issue. This makes reviewing hard.

StarpTech avatar Aug 27 '22 15:08 StarpTech

The changes are in separate commits and should be easy to be figured out.

llc1123 avatar Aug 27 '22 16:08 llc1123

The changes are in separate commits and should be easy to be figured out.

That's not what I meant.

StarpTech avatar Aug 27 '22 18:08 StarpTech

@llc1123 in order to close the issue. Please create another PR that contains only the fix for https://github.com/StarpTech/apollo-datasource-http/issues/34. The next step is to avoid a performance drop of ~15%. I'll help you with that.

StarpTech avatar Aug 31 '22 18:08 StarpTech

What's the status on this? Still chomping at the bit to cut over to this data source...

bradleymorris avatar Nov 04 '22 18:11 bradleymorris

I have migrated to apollo server v4 and this code is no longer maintained.

llc1123 avatar Nov 19 '22 17:11 llc1123