Add explicit exports for node

[pi0]

Resolves #610

By using import export condition first, we ask bundlers such as rollup to prefer an export that is compatible with native ESM (ie: directly importing in Node.js when externalized in build). Using node export condition to prefer conditions that Node.js also natively prefers in runtime.

See #610 (comment) for more context.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[timostamm]

This will cause webpack, rollup, wmr (ref), esbuild (ref) to choose "import" over "module".

I don't think we can do that, because dist/proxy/index.js is an ESM wrapper that re-exports CJS to avoid the dual package hazard. I'm pretty sure it will cause problems for tree shaking, and possibly other issues.

For reference, here is how the current exports were tested: connectrpc/examples-es#1002

I think some other solution is necessary to fix the issue, while maintaining behavior for other consumers. I'm not entirely sure which one yet 🙂

[pi0]

I'm pretty sure we can find some better solutions to support dual package hazard and replace proxy with an alternative, yet make a fully standard ESM package with a standard import export condition that works natively. (if this is your main concern to resolve) -- Feel free to direct me in discord (pi0), i can share my 50 cents of experience with dealing with ESM/CJS dual exports.

[pi0]

Updated PR as per #610 (comment) suggestion to use node export condition as a quick fix.

I also used native dist not sure if proxy is needed but let me know or feel free to directly use proxy for native node. If it is tracked with right condition, it should work.

Note: I have not e2e tested it against nitro. In theory it should work as we apply node export condition when tracing. @daviddomkar if not too much trouble, would be nice if you can check by patching in node_modules.

[daviddomkar]

@pi0 thanks for modifying the PR with a quick fix. I can confirm that when I patch the package in node_modules with changes from this PR, the problem disappears. 🎉🎉

[smaye81]

Hi, @pi0 appreciate your patience on this. We've been testing this extensively in Connect-ES also because we've noticed some similar issues with exports there. After a lot of investigation, we landed on the format specifed in this PR: connectrpc/connect-es#921.

Would you be able to modify this PR so that the exports are in the same order and configured the same way?

[smaye81]

Hi @pi0. I actually pushed up the changes to your branch, I hope you don't mind. We'd like to get this fixed in all our npm packages as soon as possible. If things seem OK to you, we can merge this branch in.

cc @timostamm

[nicksnyder]

Hi @pi0 , can you sign the CLA?

[smaye81]

Hi @pi0. We unfortunately had to land our own PR (#645) to remedy this because we wanted to close the loop. Thank you for calling this to our attention, though. Appreciate you digging into this.

[pi0]

Thanks for following and considering the change ❤️ #645 looks good!

[smaye81]

Also, FYI, this was released in v1.5.1 today.