Dual package hazard

[fubhy]

I just ran into the dual package hazard problem with @bufbuild/protobuf and the exported WKT types. I was checking instanceof on a Timestap type and that failed because the object instance I got was from the CJS export (through a CJS library) and I was comparing it against Timestamp via my local ESM import.

Personally, I think it's time to drop CJS altogether. Top-level await has been available for quite some time now and is a perfectly acceptable escape hatch for anyone who needs to import ESM modules into CJS via dynamic import.

[smaye81]

Hey @fubhy unfortunately I think this might be an issue with the ecosystem. But, can you share a bit more details with what happened? What CJS library was it that pulled in the CJS export, etc.?

Dropping CJS would be nice, but not sure we can without some tests to ensure we're not breaking a bunch of bundlers out there. I can maybe look into this with our connect-es-integration repo to see how things would behave.

[fubhy]

Hey @smaye81. Sorry for the late reply!

I can see how it might be difficult for y'all to entirely drop CJS support now that it's already "live" with both ESM & CJS.

Maybe, instead of dropping it entirely you'd consider publishing the ESM package as a CJS wrapper instead? That would eliminate the dual package hazard effectively too... Admittedly at a bit of a maintenance burden in terms additional build & build config shenanigans.

Particularly for @bufbuild/protobuf though, I think this issue can't be ignored... I ran into it in an internal monorepo with some shared dependencies between some backend & frontend services. Due to some configuration issues involving some legacy packages, the shared package with the protobuf definitions was loaded twice. Once from CJS and once from ESM.

Given how "low level" particularly the "@bufbuild/protobuf" is for anything that has anything to do with connect, I'd be willing to bet that this is destined to happen more frequently as adoption increases. Especially as people might start to publish & reuse protos & utils via shared packages. A single misconfigured (or intentionally CJS-only) package and you run into this. Mind you: this happened inside a single (admittedly large) monorepo with only local lib & app packages for me and it was already hard to debug ;-)

[fubhy]

More info on how to effectively avoid dual package hazard can be found here: https://esbuild.github.io/api/#how-conditions-work

And this PR from Mateusz adds some more too: esbuild/esbuild.github.io#40

[fubhy]

... This part in particular:

Another way of avoiding a dual package hazard is to use the bundler-specific module condition to direct bundlers to always load the ESM version of your package while letting node always fall back to the CommonJS version of your package. Both import and module are intended to be used with ESM but unlike import, the module condition is always active even if the import path was loaded using a require call. This works well with bundlers because bundlers support loading ESM using require, but it's not something that can work with node because node deliberately doesn't implement loading ESM using require.

This should work without issue or extra hassles because @bufbuild/protobuf doesn't have any default exports.

[fubhy]

@smaye81 Would you accept a PR (across all connect related repositories) that switches to ESM wrappers? That would eliminate the hazard effectively whilst also keeping support for both CJS and ESM. Unless I'm missing sth. here it would be simple to do for your packages as there's no default exports as mentioned.

[smaye81]

I'm open to a discussion on it, but let's wait until people are back from vacation, etc. (ideally next week at the earliest). Don't want to make any big decisions without a proper review.

[jcready]

Switching to ESM wrappers has the downside of effectively disabling tree-shaking as the ESM wrapper imports the CJS file. See https://nodejs.org/api/packages.html#dual-package-hazard

[fubhy]

Not if you also provide a esm version for bundlers.

[fubhy]

Basically you author the package.json something like this:

"exports": {
    ".": {
      "types": "./dist/index.d.ts",
      "module": "./dist/esm/index.js", // ESM (for bundlers)
      "import": "./dist/proxy/index.mjs", // ESM -> CJS Wrapper (proxies to dist/cjs/index.js)
      "default": "./dist/cjs/index.js" // CJS
    }
}

This way, you don't get the dual package hazard whilst retaining tree-shaking capabilities for bundlers.

[timostamm]

@fubhy, apologies for the late response. An ESM wrapper for Node.js in combination with true ESM for the module condition sounds like it would do the job.

I do wonder how well it is supported in bundlers, because the fallback-behavior would come with a pretty big bump in bundle size. Do you know any popular packages that use this setup?

[fubhy]

Hey @timostamm, no problem.

I know that @Andarist has been neck deep in this problem space. I did a quick search through his packages and found that e.g. https://www.npmjs.com/package/react-textarea-autosize uses a similar setup. This one seems to be generated through https://preconstruct.tools/

[Andarist]

Full disclosure: I use this setup in Emotion and it works exceptionally well. It's not that I didn't run into any problems though but they were really really minor and I went that extra mile to fix those issues at the root (for example in Vite). There is literally one really really edge case that I'm aware of in Next.js: vercel/next.js#49898 . It doesn't affect most applications, it's related to a very specific mix of CJS & ESM in the app code~ and so far only one project has reported it. Even though Material UI reported it - they only observed it in their own docs-related setup and it was not reported by their users. We were examining this case and it looks like a bug in Next.js and not like the issue with this setup. Other than that... I'm not aware of any issues with it :)

[timostamm]

Thanks @fubhy and @Andarist!

We just made the change for all relevant packages and ran some tests in connectrpc/examples-es#1002. It's obviously a bit difficult testing it exhaustively, but I can confirm that the popular bundlers tested resolve as expected. (Thanks to @smaye81 and @paul-sachs for testing).