search

dmonad / lib0

Monorepo of isomorphic utility functions
MIT License
347 stars 62 forks source link

Circular Dependency (encoding.js and buffer.js) #19

Closed gaberogan closed 3 years ago

gaberogan commented 3 years ago

Describe the bug There is a circular dependency between encoding.js and buffer.js. Not sure why this is necessary as encodeAny in buffer.js is the cause of this and it can easily be moved to buffer.js. That said, depending on if anyone is using this it might be a breaking change unfortunately, requiring a major release.

This isn't a big deal but does cause issues in some workflows (ie rollup).

dmonad commented 3 years ago

I'm using rollup as well for all my projects and I never encountered an issue with lib0.

Circular dependencies are fully supported by module bundlers. This is because circular dependencies are also supported by the ESM and CJS spec.

Rollup logs a warning because a circular dependencie can lead to issues. If properly applied, they do not. It is actually never a problem in pure ESM modules that only export functions / classes without inheritance.

encodeAny belongs to buffer.js and not in encoding.js, as encoding.js only contains functions that work on Encoder objects.

I think it is nice that rollup logs circular dependencies. It helps you to debug issues when issues happen. Other module bundlers don't because the whole npm ecosystem uses on circular dependencies.

Also see https://github.com/dmonad/lib0/issues/16

dmonad commented 3 years ago

Just in case you still worry: This package has 100% code coverage (every line and every branch is checked). So I would notice if this would lead to issues.

deluksic commented 1 year ago

Even though it is not a problem, would you be against extracting common functionality into a separate module just so the warning goes away?

It also makes the codebase a bit easier to understand, but I admit that's opinion-based.

szilu commented 11 months ago

I stumbled upon this issue, because I had encountered some strange issues using rollup in watch mode. I temporarily fixed the issue and it seems to be the cause. My fix was rather simple, I simply inlined all the occurences of "createUint8ArrayViewFromArrayBuffer()" to "new Uint8Array()" and dropped the buffer.js import from encoding.js and decoding.js.

Could you please fix this somehow? It's possible that this is a rollup issue, but it seems to be easily solvable.

dmonad commented 11 months ago

I feel I gave a perfectly reasonable answer to why I don't want to fix this. I like using circular dependencies as they are quite useful. Also, they are supported by the ECMAScript standard. Since lib0 has only pure exports, circular dependencies will not lead to any issues.