feat(node): support connection string syntax for nodejs client creation - #3917
Conversation
2afce42 to
e9ffac3
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3917 +/- ##
============================================
- Coverage 84.70% 84.31% -0.39%
Complexity 1398 1398
============================================
Files 1221 1222 +1
Lines 175949 174317 -1632
Branches 143047 140840 -2207
============================================
- Hits 149037 146978 -2059
- Misses 22941 23382 +441
+ Partials 3971 3957 -14
🚀 New features to boost your workflow:
|
6a3b7a1 to
44edc7c
Compare
44edc7c to
be0d997
Compare
3745728 to
3de15b0
Compare
hubcio
left a comment
There was a problem hiding this comment.
a few notes from comparing against the rust parser. one i'd want fixed before merge - the credential echo in errors.
these landed outside the diff:
foreign/node/src/stream/consumer-stream.ts:81 and :113 - singleConsumerStream and groupConsumerStream still take ClientConfig, so passing a connection string fails to compile (TS2345). they call the widened getClient, and since getClient isn't exported they're the only place users can reach the widening.
foreign/node/src/client/client.connection.ts:86-90 - reconnect timer is never unref'd and the handle isn't kept, so unlimited means the process can't exit and destroy() can't cancel a wait already in flight. the heartbeat timer next door unrefs itself with a comment saying why. pre-existing, but this PR makes unlimited a one-word option and ships it in the new e2e.
foreign/node/src/client/client.config.ts:61 - error says heartbeatInterval in raw ms at someone who typed heartbeat_interval=1000h.
nothing tests the tls branch - the pre-merge action runs tls.system.e2e.ts, which builds from an object config, so the parser never gets hit.
83122df to
8c10423
Compare
hubcio
left a comment
There was a problem hiding this comment.
round 2 - everything from the first pass landed, thanks. one new blocker though: the timer change in waitForReconnect settles the promise right away, so the reconnect loop no longer waits at all (numbers inline). two smaller things worth landing with it: the parse-duration grammar is looser than the rust one the README promises, and the new reconnect.interval check rejects configs that disable reconnect. the rest is nits.
a09ab2e to
249f21e
Compare
979889c to
e7e5d2d
Compare
e7e5d2d to
fe24bbc
Compare
Which issue does this PR address?
this PR add node sdk support for connection string syntax on client creation
accept common url like syntax
new Client("iggy://iggy:iggy@127.0.0.1:8090");note
node:urlmodule was not used since it would have introduced small differencez with rust and other sdk parsing, so connection string is manually parsed to match others sdk behaviors.