Avoid circular dependencies

[fubhy]

Fixes #644

This removes internal imports from barrel files throughout.

[CLAassistant]

All committers have signed the CLA.

[timostamm]

Hey @fubhy, while we have them, we might just as well make use of the barrels to make our own imports slightly more readable.

But the import you describe in #644 is a bug. I pushed up a commit that disallows cyclic imports via an eslint plugin.

It looks like one import got left behind 🙂

src/protocol-connect/validate-response.ts(76,22): error TS2304: Cannot find name 'Compression'

[fubhy]

Hi @timostamm! Definitely your call but personally I'd still recommend to avoid barrel files internally. Especially given the excellent code navigation tools in the IDE landscape these days I personally prefer explicit direct imports instead of opaque index.js here and there. I think for me that's actually less readable. But that might be a matter of preference indeed.

More significantly, In addition to the risk of accidentally introducing circular dependencies (which can be avoided with linting rules indeed), I've also seen weird behavior with bundlers in the past. Code splitting in particular.

[fubhy]

(That said, I'm happy to revert those changes if you'd prefer to keep the barrel file imports regardless.)

[fubhy]

TL;DR Disadvantages:

• Circular dependency hazard
• Running tests in watch mode triggers unrelated re-tests when changed
• Making it tougher for bundlers to process & optimize things (tree shaking, code splitting, etc. in particular ... currently trying to find the scenario that I ran into not long ago)
• Hazard of inadvertently imposing runtime dependencies on consumers (pulling in more than necessary) (e.g. importing something that runs code on import, like the promisify(zlib.brotli) calls, that's never used by the consuming application).
• IDEs now offer multiple options for importing the same symbol (DX suffers imho)
• Opaque index.js files with no real semantics
• Extra steps for code navigation

... That's probably not all. Will add more here if more comes to mind ;-)

[timostamm]

Make sure to run make bench, the changes will affect the code size benchmark.

Making it tougher for bundlers to process & optimize things

Would be interested to know what situation that was.

IDEs now offer multiple options for importing the same symbol.

I'm aware, believe me 😆. But we're okay as long as the DX for the user is good.

[fubhy]

I'm aware, believe me laughing. But we're okay as long as the DX for the user is good.

Painfully aware? :-P Yeah... It's not the end of the world but it does annoy me sometimes.

Would be interested to know what situation that was.

Found it. The problem was with wildcard imports / exports. Code splitting doesn't (didn't?) work with SWC in a Next.js project with those. Even with sideEffects: false. Not sure if that's fixed or if there's an inherent problem with that (or if that is / was just a bug or missing feature in SWC) but we fixed it by dropping the barrel files (although switching to named exports would've worked too). The point is that there are footguns :-P

Anyways, I'm happy with whatever you want to accept within the scope of this PR. Just thought I'd share my opinion ;-)

[timostamm]

The problem was with wildcard imports / exports

👍 I'm not sure it's relevant for users of connect-es, but it doesn't hurt to be cautious. We'd also love to idiomatically support Deno at some point, so this is the right direction.

The browserstack test failure is expected on a fork. The tests passed locally. LGTM.

Thank you for the fix 🙂