Replace callback-based API with promise-based async functions about solr-node-client HOT 37 CLOSED

I'm not concerned with breaking changes. We are moving towards the first major release, so I think we should be relentless. If we had a big software house backing the project, that would be a different story.

I'll check out undici and get back to you. I need to get this done today.

from solr-node-client.

@rudolfbyker I don't think you strictly need to use either, original implementation just takes a plain simple Request call and wraps a Duplex (which writes to request and reads from response) around it.
Now if you would use undici.pipeline, it should do all that job for you, Dispatcher.pipeline should create exactly the kind of Duplexer that we create ourselves now.
I would recommend against refactoring this part right now, though. There are no tests, so difficult to say if we break something in a major way. I suggest doing bare minimum change (replace simple Request call with simple callback-based Undici call) for now, and later on I will write a test and attempt a proper refactoring. Does that work for you?

from solr-node-client.

x2-x3 speedup in what sense? The biggest time consumers are always sending / waiting for / receiving the network response.

from solr-node-client.

I would prefer axios, because I favour maintainability. Having a well-known / popular package is better in that regard. I don't have much time left to work on this repo this month.

from solr-node-client.

This issue replaces #252

from solr-node-client.

This issue replaces #251 , too!

from solr-node-client.

@rudolfbyker See https://www.youtube.com/watch?v=tr057sP4VBM for more details on Undici and overall deep-dive into HTTP client performance.
Well, superagent is also a well-supported and popular library, we are talking 4 million weekly downloads vs 15 millions weekly downloads, so both of them are very mature and popular.

See https://github.com/fdesjardins/node-http-client-bench for (somewhat outdated) benchmarks.

I would still prefer using Undici if we are ok with a breaking change that would force people to redo mocks with builtin Undici mocking, but I would need your input on that.

from solr-node-client.

OK, I see the advantage for situations where you have many small requests in a short time span. Like when doing autocomplete queries.

from solr-node-client.

FYI nodejs/undici#1011

from solr-node-client.

Undici does not support providing a custom HTTP agent. So we will have to drop that. (If I understand correctly from the, Undici IS an HTTP agent? But I'm confused about that.)

from solr-node-client.

@rudolfbyker Yes, more or less. Since it does not use http Node module (it is more low-level than that, it operates directly on net and tls parts of Node), it does not support an http Agent either.

from solr-node-client.

Let's also get rid of the custom error types and let Undici just do its thing. Extending errors in JS is problematic at best, and so we should not force this on users of our library.

from solr-node-client.

Yeah, fair.

from solr-node-client.

I'm a bit stuck on refactoring the createAddStream method. I'm not quite sure what it's supposed to do, or how to refactor the "duplexing".

from solr-node-client.

@rudolfbyker It's a way to create a large amount of documents in bulk. Imagine that you have a billion of DB entries that you want indexed. What you might want to do is read them from DB as a stream and keep indexing them as you go.
I'm not sure you need to refactor much there, just replacing underlying Request call with Undici call should suffice. This is one case where still exposing a callback is a good idea, because that's how streams are typically consumed.

from solr-node-client.

I've never worked with streams in JS before, so I'm having a hard time wrapping my head around this. Should I be using undici.stream or unidici.pipeline?

from solr-node-client.

The thumbs up was to say that I agree with the approach in principle, but I have been at this for a long time now, and can't get it to work. Do you have time to look into refactoring the createAddStream function specifically? It could even be a separate PR on its own, while everything else still uses requests.

from solr-node-client.

Sure, I'll take a look. Can you push your branch?

from solr-node-client.

https://github.com/lbdremy/solr-node-client/compare/issue-286-promises

from solr-node-client.

I will have to stop work for today, as I'm starting to feel unwell. Thanks for the great collaboration!

from solr-node-client.

@rudolfbyker Please feel better soon!
I'll try to take it over from here. Thank you, it's fun :)

from solr-node-client.

@rudolfbyker I assume you didn't get simple Undici requests for basic operations like search to work either yet?

from solr-node-client.

from solr-node-client.

from solr-node-client.

ok, I'll avoid touching those parts yet and focus only on streams

from solr-node-client.

from solr-node-client.

can you push your half-done changes as well then?

from solr-node-client.

from solr-node-client.

thanks!

from solr-node-client.

Sorry for the delay. Pushed now. See commit messages for details.

from solr-node-client.

I'm going to take another stab at this, if you haven't started.

from solr-node-client.

not yet :). thanks!

from solr-node-client.

OK, that's all I have time for now.

from solr-node-client.

Thank you! Hopefully will be able to continue the subject this week.

from solr-node-client.

@rudolfbyker I will still add test for streaming functionality, and @AlinaKul will update the tests. Is there anything else left beyond that?

from solr-node-client.

I cannot remember. You will have to dive into this. Not sure when I will have time to continue with this one.

from solr-node-client.

@rudolfbyker Gotcha, will do! Thank you for the incredible work you've done on this!

from solr-node-client.