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

Handle multiple copies of Type due to bundling #136

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

Conversation

mabels
Copy link

@mabels mabels commented Dec 4, 2024

This change needs to be also integrated/related:
ipld/js-dag-json#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.

       to prevent problems with bundling that
       could end up with having multiple copies
       of Type implementations.
       If there are multiple copy of type the term
       xyz.type === Type.map could fail.
@rvagg
Copy link
Owner

rvagg commented Dec 4, 2024

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?

@mabels
Copy link
Author

mabels commented Dec 4, 2024

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.

@rvagg
Copy link
Owner

rvagg commented Dec 4, 2024

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.

@mabels
Copy link
Author

mabels commented Dec 5, 2024

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
Copy link
Author

mabels commented Dec 6, 2024

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

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

thx

@mabels
Copy link
Author

mabels commented Dec 13, 2024

What's your plan to land this?

cborg.js Outdated
Comment on lines 5 to 10
// is this needed for the json module and other independ encoders
export { encodeCustom } from './lib/encode.js'
export { encodeErrPrefix, decodeErrPrefix } from './lib/common.js'
export { asU8A, fromString, decodeCodePointsArray } from './lib/byte-utils.js'
export { quickEncodeToken } from './lib/jump.js'
export { makeCborEncoders, objectToTokens } from './lib/encode.js'
Copy link
Owner

Choose a reason for hiding this comment

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

this can't happen here, for two reasons:

  1. Users who don't need JSON support (a large portion of them), shouldn't be forced to get these, they should be entirely absent from the dependency tree and not add weight to a bundle if you don't use it. I know this seems to be something you're trying to solve for because you're using JSON, but this library is mainly about CBOR. So we need to find an alternative to doing this. If it means re-exporting a top-level export just for JSON that includes everything needed then that's fine.
  2. Anything exported on the top-level becomes part of the API contract and there's far too many things here that I don't want to be on the hook for supporting. Like asU8A, this is not something that I want to expose for general use, any time I want to make an internal change that involves these functions I now need to be thinking about whether it's a breaking change or not. Internal stuff must remain internal and these all look internal to me.

Copy link
Author

Choose a reason for hiding this comment

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

Thx,

I will run through it and try to move things around.
I would argue if they are used at the top-level namespace implementation the export does not reflect the bundle size.

Copy link
Author

Choose a reason for hiding this comment

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

Hi,

I did the move around --- ending up renaming lib -> cborg (which is more a hygiene thing)
I tried hard to move

  • encodeCustom
    but due to that's the real entry point for a configured encoder there is no simple way to
    no export this from the main package.
    This thing is a bit bigger than expected due to the de/encodeErrPrefix, which is now configurable.

If you are happy with that -> I will remove the comments that I left for now.

Pls do review and give some feedback. @rvagg

Due to my renaming of the directory, we should not have this PR lying around for too long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants