-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add options to configure IPFS gateway to use to fetch data #11
Add options to configure IPFS gateway to use to fetch data #11
Conversation
|
||
constructor(cache: ObjectCache, getFromIpfs?: boolean) { | ||
constructor(cache: ObjectCache, ipfsGatewayOptions?: IPFSGatewayOptions) { |
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 is a breaking change, so we should release as such.
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.
Agreed.
I'm not well-versed with the tooling around that, but saw after a quick look that the commit title feat!: <bla>
(note the bang) should be recognised as one and trigger a major version bump.
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.
Unfortunately feat!:
doesn't actually trigger a breaking change bump in the npm semantic release plugin.
You need to add a commit footer with the text "Breaking Change:", it's a pain!
Here's an example commit algorandfoundation/algokit-client-generator-ts@dd9d157
You may find it easy to squash the GitHub UI and add the breaking change info.
src/ipfs.ts
Outdated
|
||
export function getCIDUrl(ipfsGatewayBaseUrl: URL, cid: string) { | ||
const copy = new URL(ipfsGatewayBaseUrl.toString()) | ||
copy.pathname = `/ipfs/${cid}` |
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.
Not all gateways use this format, for example cloudflare used a subdomain.
I think it's probably ok to assume it's path based (not what cloudflare does), however I'd be inclined to require the full url minus the cid, so in the default value that'd be https://ipfs.algonode.dev/ipfs
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.
Understood, thanks for the feedback, let me change that.
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.
Done in 68bcaaa.
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.
Cloudflare actually still worked with this format
BREAKING CHANGE: The `CacheOnlyIPFS` constructor allows specifying which IPFS gateway to use to fetch assets.
Description of change
This adds options to
CacheOnlyIPFS
,PinataStorageWithCache
, andPinataStorage
to specify which IPFS gateway to use when fetching data.Options include the base URL of the gateway (assuming the content can always be fetched at
/ipfs/<cid>
).Pull-Request Checklist
main
branchnpm run lint
passes with this changenpm test
passes with this changenpm run commitlint
passes with this changeFixes #0000
Please note that this repository uses conventional commits in combination with semantic-release to automate package publication. Therefore, your commit messages are critical, and the build process will lint them. If the build fails at the commitlint stage, please refer to this handy guide on how to update the commit messages