-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
0.10 dag API changes breaking HTTP API (discussion for resolution) #8415
Comments
#8416 has option (b) for issue (3) |
For (3) I'm not convinced that the changed names is great either, because it introduces a second breaking change of that API. It seems easier to explain the 1 change, rather than making people who are using this functionality to migrate twice. For (1) I think we should continue pushing for cid v1 as the standard. |
I think the approach I'm suggesting with #8416 reduces the breakage surface area here to nearly nothing rather than introducing something else that breaks. Keep the old behaviours in place where they are exclusively used but add affordances for new behaviour and encourage (via docs and perhaps hiding the CLI option) use of the new pattern. The question of removing deprecated APIs I think is separate and I see a lot of them in here already so I don't imagine that they're going to be gone any time soon. I don't have a very strong preference for #8416, it'd just be nice to avoid impacting users so overtly and now we know that there are common call paths that are going to break, maybe we have the opportunity to be a bit more gentle. Not my call, just presenting options. |
(i think you can ignore this point towards (3). I had incorrectly assumed v0.10 was already final, but it sounds like it's still in release candidate mode. I'm okay either way given that.) |
Something to note here is that we will end up making some breaking changes to the block API at some point having to do with the names being incorrect/inconsistent. e.g. protobuf instead of dag-pb, or more terribly cbor instead of dag-cbor and json instead of dag-json.
I don't see a good reason to keep the CID version. If someone really wants to convert a CIDv1 to a CIDv0 they can use the
Yep. We now allow all IPLD codecs to be used as input and output formats. Should be documented as a new feature.
I see the argument here, although I'm a little concerned that while we could make things "work" it'll end up being more confusing. For example, we previously had So IIUC this change only helps if:
So is this only helping people who previously had Basically, what I'm thinking is that given all the changes made in the On that note, one of the commands I'm currently worried about is WDYT @rvagg |
WDIT? Diabolical choices! I think this really depends on expected user behaviour currently in the wild, which I have no data on, nor much experience with. It's just interesting to see OribitDB running into this and our own js-ipfs-http-client doing it too, suggesting that maybe it's not that rare? Given the patterns I've seen in both of those, I'm imagining Of course one option here is to just properly break it and rename |
@rvagg that's a good point about the breaking nature of It seems like it might be a better idea to just replace |
@aschmahmann : I agree with your proposal as it causes folks to fail loud/fast/hard. |
+1 to
Except in the case of bytes, which now uses the dag-json bytes layout. Probably worth making that clear. There's also the edges with numbers—where we now have differentiation between integers and floats, v0.9.1 and prior would interpret all json input numbers as floats. I'm not sure we documented that in the changelog yet either. The only other thing to consider is the ipfs-http-client stuff; it'll be a hard break there. I know js-ipfs-http-client hard-wires defaults for both input-enc and format, so there'll be a version break for people trying to use it and the newer go-ipfs. I'm not sure that can be easily finessed with version sniffing or not but it might be awkward. |
Re: producing CIDv1 +1 I am also +1 for renaming and forcing people who would be impacted to notice the change. "
|
Surfaced via @tabcat's work @ orbitdb/orbitdb#909, picked up in failing tests @ https://github.com/ipfs/js-ipfs/blob/870d446f/packages/ipfs-http-client/test/dag.spec.js when running against the
go-ipfs@next
package which is the 0.10.0-rc1 binary.In summary - the changes we made are fine, there's no error, but we may have gone harder than we should have, or intended to and there are ways we could bridge the breakage for users. @tabcat says they're moving to the block API exclusively, which is fine and probably suits the workflow there, but the dag API is where we've put all the new goodness and users avoiding it suggests that maybe we could do better.
cid-version
fordag put
, or at least it doesn't work.Looking through history I'm wondering if we ever had a
cid-version
for the dag API? It looks like it's only valid for theadd
andfiles
API. Does this mean that https://github.com/ipfs/js-ipfs/blob/870d446f/packages/ipfs-http-client/test/dag.spec.js#L33 is incorrect and is only working because it's defaulting to v0 for a dag-pb block?Maybe the resolution here is to be clear in the CHANGELOG that the CIDs for the dag API are always v1? I don't see a note for that anywhere. Alternatively we could just introduce it there and allow it to be used for the case where
format=dag-pb
so people can get their beloved CIDv0's.git-raw
codec is included by default now.This test https://github.com/ipfs/js-ipfs/blob/870d446f/packages/ipfs-http-client/test/dag.spec.js#L77 assumes it's not available, but it gets an answer. Are we intending to enable the plugin by default now?
input-enc
andformat
might make more impact than we intend.We now have the very clear and crisp meaning that
input-enc
means the codec used to decode the bytes that you send in, andformat
is the codec that it should use to re-encode the node after it's been decoded. The two arguments are independent variables now. Prior to this updateinput-enc
was more subtle and tied to theformat
argument. It meant something like "I'm giving you data that I want stored informat
codec, and I'm either giving you theraw
bytes of the data already in that codec, or ajson
form that will unmarshall into a node that you can then encode in that codec". That's about it. Each of theraw
,cbor
andprotobuf
options forinput-enc
simply say "I'm giving you raw bytes" and then narrow down yourformat
options to match (exceptraw
which has the full flexibility). Thejson
input-enc
option does its unmarshall routine, with its pseudo-dag-json scheme.So, if you look at https://github.com/ipfs/js-ipfs/blob/870d446f/packages/ipfs-http-client/test/dag.spec.js again you'll see that behaviour in action.
input-enc=raw format=dag-cbor
means "I'm giving you some rawdag-cbor
encoded bytes that I expect you to store". But now we're saying that it means "I'm giving youraw
data, which is just a single byte array, that you should encode as a dag-cbor, i.e. a single byte array prefixed with a CBOR tag.".Possible actions with this could be:
a. Be much more clear in our documentation about this change and what it means now.
b. Deprecate
input-enc
and introduce a newinput-codec
orinput-format
to do what we're now doing with it. We just assume that the bytes being provided can be passes straight to a decoder forinput-codec
unlessinput-enc
isjson
, in which case we have a conflict and either override withinput-codec
if supplied, or if not supplied, setinput-codec
todag-json
. I think that would give a pretty clean upgrade path without breaking existing use-cases.@aschmahmann @achingbrain @warpfork @willscott
The text was updated successfully, but these errors were encountered: