fix: make ipfs the default 421 proto name #77
Conversation
ghost
assigned 
src/protocols-table.js
Outdated
| [421, Protocols.lengthPrefixedVarSize, 'p2p'], | ||
| // preferred name for 421 (added below p2p, ipfs takes precedence during table population) |
There was a problem hiding this comment.
This comment isn't correct anymore.
There was a problem hiding this comment.
I did an update because I saw it wasn't correct when I made the change and fixed the comment. The push may have just taken a little bit as I'm on a train 🚆.
There was a problem hiding this comment.
I find it confusing (as you can see on my comments). Why not putting a comment before the p2p saying:
// `p2p` is the preferred name for 421
And before ipfs:
// `ipfs` is intentionally below `p2p` so that `ipfs` takes precedence during table population
// this enables us ...
Perhaps in nicer English, but you get the point :)
There was a problem hiding this comment.
Updated, let me know if that is clearer.
|
This is a good way to do it - introduce support and then change the default. We're doing the same thing with CIDv1 and base32 :D |
|
All Node.js tests passing locally for me in js-ipfs with this PR - I'd say this is good to go! 👍 |
|
Thanks @alanshaw and @jacobheun for getting fixing this o quickly. I'll do another release. |
|
New release is out! |
This corrects an issue with the order of the protocol table. Previously p2p was set last in the order, which caused all created multiaddr's to use p2p instead of ipfs. While
p2pis the desired protocol name, this helps better support legacy systems that will not yet have support for p2p.Eventually,
p2pshould be transitioned to the default.I've updated the test suite to account for p2p and ipfs protocol checks. They verify that created multiaddr's will always use ipfs instead of p2p.
Reference #76 (comment)