Skip to content
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

multicodec v4.0.0: generate JavaScript exports from CSV #83

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

darrylyeo
Copy link

@darrylyeo darrylyeo commented Jun 9, 2023

Revives the previously deprecated multicodec npm package as discussed here.

@darrylyeo darrylyeo changed the title multicodecs v4.0.0: generate JavaScript exports from CSV multicodec v4.0.0: generate JavaScript exports from CSV Jun 9, 2023
@whizzzkid
Copy link

Thanks for PR @darrylyeo I am not sure if I am understanding this correctly, is this something that is not offered by https://github.com/multiformats/js-multiformats at the moment? Also, why not improve https://github.com/multiformats/js-multiformats as this package has been deprecated?

@vmx
Copy link
Member

vmx commented Jun 21, 2023

@whizzzkid as mentioned in description, this PR was triggered by multiformats/multicodec#327 (comment).

"description": "JavaScript implementation of the multicodec specification",
"leadMaintainer": "Henrique Dias <hacdias@gmail.com>",
"main": "src/index.js",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line needs to be changed to dist/src/index.js ?

Also probably nice to leave the "types" field.

Also worth considering or investigating whether aegir will build ESM or commonjs by default and ensuring that the desired module type is output.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"types"

@whizzzkid
Copy link

@vmx understood, however I still feel reviving this repo will create a fork in the initial thought process. Won't making this part of https://github.com/multiformats/js-multiformats/tree/master#implementations make this a lot more simpler and maintainable?

Which has been addressed by:

However, with the growth of the table, we've mostly been recommending people use separate packages just for the codecs (addrs, etc.) that they need, rather than consume the entire table since it can really bloat dependency trees and bundles. It's typically much better to just target what you need for an application

But if the team feels it's the right way, I'm not opposed to this!

@rvagg
Copy link
Member

rvagg commented Jun 23, 2023

@whizzzkid can you explain a bit more about why you think this might be a problem? My thought here is that there is a genuine use-case to having a JavaScript consumable form of the table. Just the table, no other frills. And it looks to me like that's what @darrylyeo has done here. Documentation here should be very clear that this is not a common use-case, that people should not reach for this unless they need access to the whole table, or large slabs of it.

I think we hit the right approach with js-multiformats, keeping it lean and minimal, but we do have a problem of codes being scattered and you have to trust the package that you're importing that it's doing the codes right; this isn't normally a problem, but there are cases where a stronger relationship to the multicodec table is very helpful. It's kind of like having applications deal with their own mime type—both publishing and consuming their narrow concern, but a /etc/mime.types sure is helpful sometimes when you have cross-cutting use cases.

Lastly, to anticipate a possible suggested approach - I don't support a version of this where the table goes in to js-multiformats. Bloat in that package is something I feel fairly strongly about, it's at the core of our JS stack and needs to stay light and lean, not just in someone's direct require/import path to make their bundles small, but also on-disk since it shows up in everyone's node_modules for every single JS package. js-multiformats has its genesis in bloated core packages, where the majority of people were burdened with code they never touched (both on disk and in bundles!). The table is not a majority concern, so it makes sense to shunt it off to the side I think.

@whizzzkid
Copy link

Thanks for the insights @rvagg, there are multiple reasons why I feel this is not the right approach.

  1. Legacy, there are multiple articles/tutorials/historical examples that point to multicodec, which promises it does one thing, but after this it does not do those things. The burden transfers over to the developer to validate from the documentation on what this package does. The state of js package managers is always, weird, some random project that always like to update to latest might break because we changed this.
  2. Overloading, As you said, this is not a common use-case if that's correct and this is seldom needed, then I'd prefer this to exist as a new package multiformats-lookup or multiformats-table. Repurposing existing package for this may not be ideal, the deprecation notice will now have to exist inside the documentation and js package managers will not notify users about deprecation.
  3. Contradictory sources of truths, you have to trust the package that you're importing that it's doing the codes right well if we can't trust the package to do the codes right, then can we even trust the package? If the devs need one package to validate another package, that will only create dissonance. Which package is in the right? If the dev needs to override something that's not done correctly in one package, I'd assume they'd do it themselves instead of equipping them with another source of truth that they now have to trust. There should only be a single source of truth.

Then again, personally I feel it should be ok as I know this change happened, but this might have a larger impact on the community which is not measurable at this point, and may set a bad precedent.

I'd also let @hacdias chime in to see what are their thoughts about this.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on reusing deprecated NPM package feeling fishy.

If I understand correctly, legacy code, examples, and tutorials, stackoverflows, and who knows what else may still reference the deprecated multicodec NPM package. If someone uses that, they get a useful warning to switch to a modern replacement.

If we un-deprecate it, people will not get that, they will not know they should use the multiformats NPM package instead.

Wouldn't it be safer if a new repo and NPM package was created for this, like multicodec-table?
Any reason we have to use same repo and NPM name? (apologies if I missed some prior conversation, just a drive-by comment)

Copy link
Member

@lidel lidel Jun 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine this custom action should be replaced with our generic releaser from https://github.com/protocol/.github/blob/master/configs/js.json
See https://github.com/multiformats/js-multiaddr/tree/master/.github/workflows for prior art.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I requested this one be put here, it's based on the one @galargh helped construct over at https://github.com/multiformats/go-multicodec/blob/master/.github/workflows/go-generate.yml

if we want to make that more generic in protocol/.github then that's fine, but in lieu of a cron scheduler workflow to check for updates to multiformats/multicodec then this is probably what we need here.

@rvagg
Copy link
Member

rvagg commented Jun 27, 2023

Yeah, I buy the argument re package naming and all the existing documentation that points to this package being a problem. Let's just do this under a different name then. I'll make a js-multicodec-table repo and we could publish it as @multiformats/multicodec-table, nice and verbose and unfriendly to the uninitiated.

@rvagg
Copy link
Member

rvagg commented Jun 27, 2023

@darrylyeo see https://github.com/multiformats/js-multicodec-table, I've added you as a maintainer so just open this PR (ideally without the js-multicodec history) over there and we'll get it rolling. Make the package name @multiformats/multicodec-table.

@BigLep
Copy link

BigLep commented Sep 5, 2023

@darrylyeo: are you going to finish this off by renaming the package?

@BigLep BigLep added the need/author-input Needs input from the original author label Sep 5, 2023
@@ -1,4 +1,5 @@
package-lock.json
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
package-lock.json
package-lock.json

@@ -1,4 +1,5 @@
package-lock.json
pnpm-lock.yaml
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pnpm-lock.yaml
pnpm-lock.yaml

@@ -1,4 +1,5 @@
package-lock.json
pnpm-lock.yaml
yarn.lock
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
yarn.lock
yarn.lock

@@ -1,4 +1,5 @@
package-lock.json
pnpm-lock.yaml
yarn.lock

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

@@ -1,4 +1,5 @@
package-lock.json
pnpm-lock.yaml
yarn.lock

**/node_modules/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
**/node_modules/
**/node_modules/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/author-input Needs input from the original author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants