-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow to provide node address as array for fallbacks #418
Conversation
src/dock-api.js
Outdated
? new WsProvider(this.address) | ||
: new HttpProvider(this.address); | ||
? new WsProvider(addressArray) | ||
: new HttpProvider(addressArray); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HttpProvider
doesn't accept an array as per its docs. Have you checked that it works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just updated this, thought maybe we had a test for it (we should at some point) but doesnt look like it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created #419 for testing it in the future
Signed-off-by: Sam Hellawell <[email protected]>
@@ -99,10 +104,16 @@ export default class DockAPI { | |||
rpc = Object.assign(rpc, PoaRpcDefs); | |||
} | |||
|
|||
const isWebsocket = this.address && this.address.indexOf('http') === -1; | |||
const addressStr = addressArray[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The correct way to handle would be to raise error if a mix of URL types is provided or accept a preference of websocket vs http. This requires a non-trivial amount of work so approving it for now. Can you add a note here please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah noticed that too, i was thinking of looking into a way to mix websocket/https urls if possible but im not sure polkadotjs allows this. essentially creating a provider per type/url
No description provided.