cborg icon indicating copy to clipboard operation
cborg copied to clipboard

Handle multiple copies of Type due to bundling

Open mabels opened this issue 1 year ago • 7 comments

This change needs to be also integrated/related: https://github.com/ipld/js-dag-json/pull/134

Why this fix is important if cborg is used, for example, esm.sh due to implicit bundling it could occur if cborg and cborg/json are used in the same project there will be two copies of token.js one in cborg and another one in cborg/json. The implementation uses terms like: xyz.type === Type.map they could then fail because the type from xyz is used from cborg/json or @ipld/dag-json, and the import Type is coming from cborg.

I added an export to cborg named json to prevent any issues like that. For the cborg/json export, I tried to make a forwarding package to prevent that problem. This needs further tests, and if they are successful, it should also be used for taglib and length. To eliminate that problem, it might be better to drop those exports.

To conclude this problem, if exports are used, they have to be self-contained and not reference any "local" code like ../token.js.

mabels avatar Dec 04 '24 09:12 mabels

Oh, this is a lot, but I'm glad you went with a new equals because that was the thing that stood out to me on the dag-json PR. Another quick drive-by while I'm glancing at this (I'll try and get back to this tomorrow) is that I'm not super keen on including the JSON stuff on the main export. You shouldn't have to include any of the JSON code in a bundle if you're not intending to use it. Is there another way we can achieve what you want? Maybe having an alternative top-level export just for JSON consumption?

rvagg avatar Dec 04 '24 11:12 rvagg

Thx for the quick reaction. To the json export, I'm also not happy with the solution.

  1. If there is used Type.equals than the double import will hopefully not cause any trouble.
  2. The json export could be dropped if we are successfully test the forwarding package. Which needs some rework -- I will try it soon. So the json export should import cborg as an external. I'm not sure if esm.sh could do that.

To do these tests I need first to try to integrated the patched cborg (which does not exist for now) into my app and that's some kind of complex I have to replace cborg dependency in a nested tree and that with esm.sh.

@ipld/car 5.3.3
├─┬ @ipld/dag-cbor 9.2.2
│ └── cborg 4.2.6
└── cborg 4.2.6
@ipld/dag-cbor 9.2.2
└── cborg 4.2.6
@ipld/dag-json 10.2.3
└── cborg 4.2.6
@web3-storage/pail 0.6.0
└─┬ @ipld/dag-cbor 9.2.2
  └── cborg 4.2.6
cborg 4.2.6
ipfs-unixfs-exporter 13.6.1
├─┬ @ipld/dag-cbor 9.2.2
│ └── cborg 4.2.6
└─┬ @ipld/dag-json 10.2.3
  └── cborg 4.2.6

After my env setup I should be able to verify if the forwarding packages are working.

mabels avatar Dec 04 '24 12:12 mabels

OK, crazy dep tree; I don't know about esm.sh but I gather the problem you're facing is that it's not deduping these for you like it probably should? Reasonable ask that it should be instance-independent though, I'm just surprised to see no deduping going on here where it seems like a no-brainer.

rvagg avatar Dec 04 '24 23:12 rvagg

About esm.sh and jsr.io both are services from cloudflare and deno. Which enable you to use your software from your browser directly without any build step. Like do:

import { decode } from 'https://esm.sh/cborg/json'

This is nice but I don't see how they could solve the dedubbing: If in the json module a local reference like:

import { Type } from '../token.js'

is used. Anyways I'm almost done with my vendor package, hopefully there will be some result in a few hours.

mabels avatar Dec 05 '24 07:12 mabels

I removed the json namespace and tested the build with esm.sh

https://esm.sh/v135/@fireproof/[email protected]/cborg/json

thx

mabels avatar Dec 06 '24 08:12 mabels

What's your plan to land this?

mabels avatar Dec 13 '24 21:12 mabels