Consistent naming and export conversion tables

[hacdias]

This PR aims to bring some naming consistency and avoid breaking anything else in the future with internal changes. I made separate commits as the last one might be controversial, or we can decide to release it separately. I deduped code and simplified it. The negative coverage diff is caused by unused functions in the utils.js file which is never tested. Since we have less code, that function accounts for a higher coverage percentage.

By commit, this is what I did and why:

bcdb9be - rename CodecNumber to CodecCode

This was a matter of making the names consistent with the "source" table and use the nomenclature "name" and "code" instead of number.

be1a57f - export tables and dedup code

• Renamed the file base-table.js to generated-table.js to avoid confusions in the future.
• Created a file maps.js which builds and exports the following maps:
  • nameToVarint
  • constantToCode
  • nameToCode aka the base table.
  • codeToName
• Exported all the tables above in index.js so developers can directly use the tables if they prefer over the functions.

e93b439 - rename functions

The controversial one, the least needed and the most breaking. I renamed the functions to make them consistent and actually have meaningful names. Looking at some names, I wouldn't be able to guess what they did. I think this is very important for newcomers or even just for clarity of code.

• Renamed getCodec to getNameFromData
• Renamed getName to getNameFromCode (you see? By their previous names, both could be either thing)
• Renamed getNumber to getCodeFromName
• Renamed getCode to getCodeFromData
• Renamed getCodeVarint to getVarintFromName
• Renamed getVarint to getVarintFromCode

60d65df - getVarintFromCode returns Uint8Array instead of Array

To make the getVarint functions consistent, they both now return Uint8Arrays instead of getVarintFromCode returning an array. I'm not sure about the implications of this. I think the result is correct, and it is just a breaking change.

Things I thought of but didn't do

I thought about merging getNameFromData and getNameFromCode into getName and check by the data type, instead of exporting all variants. The same for the getCode and getVarint functions. I don't know if that's prefered over having such verbose functions. Please let me know what you think.

---

Please review everything and let me know what you think, or how you think we should proceed.

License: MIT
Signed-off-by: Henrique Dias hacdias@gmail.com

[hacdias]

Or export them as table: #71

[rvagg]

At a high level this seems good to me. We recently have had discussions about the naming and I think came to the conclusion that "code" was preferable, but @vmx is going to have a clearer mental picture of it all since he's been taking on naming in Rust while thinking about these things.

[vasco-santos]

This LGTM

• renames seem helpful. They increase the API surface, but I would rather have them this way than accepting different input types and verification
• export tables and maps 👍🏼
• getVarintFromCode returns Uint8Array, I have no strong opinion here as I am not sure where this is used. However, I think we should always be consistent in APIs, and users can always convert this.

Nice summary of the changes, once you release this it would be good to add all this information in the Changelog + Release notes for easing the migration

[mvdan]

I agree with the changes, in general. The recent multicodec naming discussion happened because I wanted to revive go-multicodec, and I wanted to get the names reasonably right. Here's the result: https://pkg.go.dev/github.com/multiformats/go-multicodec#pkg-index

In summary, a Code type with many exported constant values (think like an Enum), one per table entry. Then, a String method on the type to convert a constant to its name string. So it's very similar to your CodecCode (but minus Codec), and the String method is like getNameFromCode. I think we also agreed that the name "codec" shouldn't be used in general, because the vast majority of the codes don't describe codecs. In a way, the table should perhaps have been named "multicode table".

I can't really comment on the rest of the API surface. At least for Go, it felt largely unnecessary. We might the equivalent to getCodeFromName in the future, but it didn't feel like a must-have.

[vmx]

If I look at this library in isolation, those changes make sense. Though if I look at the whole ecosystem, I have concerns about making such a huge breaking change.

js-multiformats is working differently than the current multiformats implementations. It doesn't depend on such a central table anymore. Internally it just uses the code and not e.g. the string identifier. I guess sometimes you might want to use such a table though, e.g. in order to get that code.

Though it might make sense wait and see how/if people are still using js-multicodec even when they are using js-multiformats.

As the IPLD team would really like to see the move to js-multiformats, I'm not sure if we should introduce major changes to the current implementations. This would mean that folks need to fix breaking changes and later we tell them to a completely new stack. On libraries.io it currently lists 74 modules using js-multicodec.

I propose spending time rather on making upgrades to js-multiformats smooth/helping with the upgrade, rather than breaking existing APIs.

[achingbrain]

I propose spending time rather on making upgrades to js-multiformats smooth/helping with the upgrade, rather than breaking existing APIs.

I think this would be preferable and would cut down on the amount of work created by this sort of change.

[Gozala]

I’m with @vmx on this, would prefer to do this post, or better yet, in concert with switch to js-multiformats so it’s not breaking change after breaking change. Ideally we could also have a transition phase where old names are still there, but deprecated, emitting warnings about upcoming breaking change and how to update

Edit: Elaborate a bit on deprecation note above

What I propose specifically to smooth out the transition is following:

1. Keep the old names in place, but just make them delegate to the new ones.
2. Mark old names with @deprecated that shows up in vscode & lints signaling users that they are deprecated.
3. Make old functions (and non functions through getter access) issue a console.warn with a message about it deprecation and a new alternative method to use for this functionality & a note on when the old names are going to be removed (I propose next major version bump)
4. Optional: Deprecation can start throwing same message instead of console.warn.
5. In the next of next major fully remove deprecated names.

I would really like if we used this general approach for rolling out breaking changes as this gives users some room to plan the work needed to upgrade as opposed to having to stay behind or update.

With all that said, I still think it would be good to coordinate this with js-multiformats transition given that it eliminates need for names and codes and uses references to the codecs instead.

[hacdias]

According to all your comments and suggestions, I refactored this PR into 3 main commits. Please look at all them. They all have descriptions. Basically, I kept the old names as alias, and getVarint returns a regular Array for backwards compatibility.

I don't think there are any breaking changes here. Does changing the type name from CodecNumber to CodecCode break anything else?

The coverage is much lower because it doesn't test the alias. Should they be tested too?

This will need to be released as a major anyways due to #72 unless it is reverted.

[hacdias]

Pinging @Gozala and @vmx

[vmx]

I'm OK with those changes. I think it would be great to have them tested with js-ipfs before doing a release.