TypeScript resolves CJS type definitions for ESM code with `NodeNext` module resolution about protobuf-es HOT 5 OPEN

Hi @haines I dug into this a bit and I was able to recreate as you mentioned. The issue seems to be the two different module/moduleResolution combinations. If I switch both app and lib to use NodeNext/NodeNext or if I switch both to use ESNext/Bundler, it works fine. I'm not entirely sure why TypeScript is resolving the CJS export path however.

I know you linked a TypeScript issue above, but I'm not 100% sure it's the same problem. Would you be willing to file an issue on TypeScript's repo and post your repro there to see if they have any feedback or opinion on what's happening? If/when they reply, we can go from there on what the best approach is.

from protobuf-es.

Hi @smaye81, I think you're right, this is not due to that TypeScript bug.

It's because of the node export condition in package.json.

I used patch-package to double-check exactly which imports were resolved and I was slightly surprised by the answer.

https://github.com/haines/buf-cjs-esm-ts/tree/check-which-types

==> ESNext/Bundler
loaded CJS
loaded proxy
loaded ESM types

==> NodeNext
loaded CJS
loaded proxy
loaded proxy types

So, at runtime, Node loads the proxy module as specified here, and thus ultimately loads the CJS code.

TypeScript, however, ignores the node condition in ESNext/Bundler mode and loads the ESM types by following the import condition here.

In NodeNext mode, it follows the node condition and thus loads the proxy types (which just re-exports the CJS types). This is why the type mismatch occurs in a mixed project.

Perhaps the proxy should be re-exporting the ESM types instead?

Although, why is the node condition necessary at all? I don't really want to load the CJS code in a modern ESM-supporting version of Node.js.

from protobuf-es.

We are actually going to be fixing this in a future release of Protobuf-ES (see issue #713). The plan will be to remove the node condition and the module condition entirely. However, when investigating with your repro, I also tested with this change in place and basically saw the same behavior. I still got the TypeError that you noted above.

The one difference though was that if I flip-flopped your settings (app using NodeNext/NodeNext and lib using ESNext/Bundler), the build process succeeded if I tested with this change in place. If I flip-flop these settings and test with the current release of Protobuf-ES, I see the same TypeError, only with the module format flip-flopped (CJS is not assignable to ESM).

This is where things get murky. I really have no idea how TypeScript is resolving these under the various circumstances. That's why it may be worth posting something on their repo to see if there's some fundamental issue with our setup or if this actually is a bug in their resolution logic.

from protobuf-es.

We are actually going to be fixing this in a future release of Protobuf-ES (see issue #713).

Nice, makes sense.

However, when investigating with your repro, I also tested with this change in place and basically saw the same behavior.

Likewise. I have boiled it down to a fairly minimal reproduction (https://github.com/haines/typescript-dual-package-esnext-bundler-nodenext); it doesn't seem to be anything specific to your setup. I've raised microsoft/TypeScript#57553.

Maybe mixing module/moduleResolution settings is a bad idea but it seems reasonable to me for our setup and isn't obviously verboten in the TS docs 🤷‍♂️ Hopefully someone will have some insight over there... this whole dual-package / ESM / CJS thing is a nightmare 😢

from protobuf-es.

Great! Nice work on the repro. It looks like they agree it's probably a bug. Will be curious to hear what's going on.

from protobuf-es.