search

StarpTech / apollo-datasource-http

Optimized JSON HTTP Data Source for Apollo Server
MIT License
73 stars 32 forks source link

Memoize promises instead of results to avoid concurrency problems #37

Closed llc1123 closed 2 years ago

llc1123 commented 2 years ago

Fixes #34

llc1123 commented 2 years ago

Anything blocking this PR? @StarpTech

StarpTech commented 2 years ago

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

tdipadova3rd commented 2 years ago

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

StarpTech commented 2 years ago

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

llc1123 commented 2 years ago

@StarpTech fixed.

bradleymorris commented 2 years ago

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

StarpTech commented 2 years ago

Did you run the benchmarks?

llc1123 commented 2 years ago

Updated. @StarpTech

tdipadova3rd commented 2 years ago

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

0xTea commented 2 years ago

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

same , waiting on this update 👍

tdipadova3rd commented 2 years ago

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

StarpTech commented 2 years ago

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

StarpTech commented 2 years ago

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

llc1123 commented 2 years ago

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

StarpTech commented 2 years ago

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

That's not what I meant.

StarpTech commented 2 years ago

@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.

bradleymorris commented 2 years ago

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

llc1123 commented 2 years ago

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