fix: enable preload on MFS commands that accept IPFS paths #2355
Conversation
fixes #2335 License: MIT Signed-off-by: Alan Shaw <[email protected]>
src/core/components/files-mfs.js
Outdated
| @@ -32,15 +34,37 @@ module.exports = self => { | |||
| repoOwner: self._options.repoOwner | |||
| }) | |||
|
|
|||
| const withPreload = fn => (...args) => { | |||
| const cids = args.reduce((cids, p) => { | |||
| if (isIpfs.ipfsPath(p)) { | |||
There was a problem hiding this comment.
IIRC /ipns/ paths are not supported by files.stat.
That is why HTTP gateway resolves /ipns/ → /ipfs/ before passing it to stat:
js-ipfs/src/http/gateway/resources/gateway.js
Lines 40 to 46 in 3cade67
That being said, is it possible /ipns/ paths will be supported in the future, as suggested in ipfs/js-ipfs-http-response#22? If so, a future-proof check would be:
| if (isIpfs.ipfsPath(p)) { | |
| if (isIpfs.path(p)) { |
There was a problem hiding this comment.
I think we'd also have to add the resolve step to this PR if we wanted to "future-proof" the check...right now it would end up sending Peer IDs to the preload nodes...
There was a problem hiding this comment.
No, preload call for /ipns/ path will work fine. Check with:
$ curl "https://node0.preload.ipfs.io/api/v0/refs?arg=/ipns/docs.ipfs.io&r=true"Above is DNSLink, however I think preload of real IPNS (with peerid) will be actually beneficial by making the key available in more places.
IPNSsupport does not need to land/block this PR. Let's merge is asap.
License: MIT Signed-off-by: Alan Shaw <[email protected]>
|
Note: Tests are all passing on CI. The |
There was a problem hiding this comment.
LGTM, works as expected.
Sidenote: do we know what will be the impact this change on default preload nodes? Watching logs when running DEBUG="*preload*" jsipfs daemon and then opening http://127.0.0.1:9090/ipns/tr.wikipedia-on-ipfs.org/wiki/Mars.html is pretty intense.
src/core/components/files-mfs.js
Outdated
| @@ -32,15 +34,37 @@ module.exports = self => { | |||
| repoOwner: self._options.repoOwner | |||
| }) | |||
|
|
|||
| const withPreload = fn => (...args) => { | |||
| const cids = args.reduce((cids, p) => { | |||
| if (isIpfs.ipfsPath(p)) { | |||
There was a problem hiding this comment.
No, preload call for /ipns/ path will work fine. Check with:
$ curl "https://node0.preload.ipfs.io/api/v0/refs?arg=/ipns/docs.ipfs.io&r=true"Above is DNSLink, however I think preload of real IPNS (with peerid) will be actually beneficial by making the key available in more places.
IPNSsupport does not need to land/block this PR. Let's merge is asap.
|
While testing this PR in Brave I noticed multiple requests for the same CID blocking browser's request queue. Potential mitigations:
|
License: MIT Signed-off-by: Alan Shaw <[email protected]>
There was a problem hiding this comment.
Tested fixup with Brave, preload works as expected now 👍
FYSA: preload of sharded paths such as /ipns/tr.wikipedia-on-ipfs.org/I/m/Mars_Hubble.jpg returns ERROR 500 but that should get fixed when preload nodes are updated to go-ipfs with the fix from ipfs/kubo#6601
@achingbrain please could you verify these are the methods that accept IPFS paths? Also, do they accept CID instances or CID strings or "cid paths" as I've allowed here or is it only IPFS paths?Verified myself - it is IPFS paths (Strings) and CID instances.TODO:
fixes #2335