-
Notifications
You must be signed in to change notification settings - Fork 134
fix: propagate provider option through session and broker to trustles… #833
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
base: main
Are you sure you want to change the base?
Conversation
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.
I believe this can be replaced by #834?
|
|
||
| try { | ||
| const block = await gateway.getRawBlock(cid, options) | ||
| const block = await gateway.getRawBlock(cid, { ...options, provider: options.provider }) |
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.
| const block = await gateway.getRawBlock(cid, { ...options, provider: options.provider }) | |
| const block = await gateway.getRawBlock(cid, options) |
| /** | ||
| * By default we will only connect to peers with HTTPS addresses, pass true | ||
| * to also connect to HTTP addresses. | ||
| * to also connect to HTTP addresses. |
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.
Please don't change lines unnecessarily.
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.
Thanks for opening this.
Please address all comments and add tests to prevent regressions.
It cannot be merged without tests.
| * @default 2_097_152 (2MiB) | ||
| */ | ||
| maxSize?: number | ||
| provider?: string |
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.
This should be providers?: string[] to better align with the option added in #834
Please also add a jsdoc comment explaining what this option does.
| gwUrl.pathname = `/ipfs/${cid.toString()}` | ||
|
|
||
| // necessary as not every gateway supports dag-cbor, but every should support | ||
| // sending raw block as-is |
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.
Do not remove this comment
| * @default 2_097_152 | ||
| */ | ||
| maxSize?: number | ||
| provider?: string |
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.
This should be providers?: string[] to better align with the option added in #834
Please also add a jsdoc comment explaining what this option does.
|
I'm converting this to a draft as it's blocked on ipfs/specs#504 |
Ensure the
provideroption is correctly propagated from the API, through the session and broker layers, down to the trustless gateway HTTP request.Fixes #816