-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ESM wrapper to avoid dual package hazard #180
Conversation
@paul-sachs, do you have an idea what the CI failure is about? I just ran |
@timostamm hmm I'll take a look. I've sometimes run into this if my branch is out of date (github compares using a merged branch). |
Ah, i see what's causing the issue. turbo is running test, and for some reason, it thinks it needs to run the generate step again (since it says cache has been missed). And the generate step doesn't run prettier as part of formatting. Why this isn't happening locally I don't know. I'll dig in some more and see if I can refine these dependencies. |
Alright, i figured it out. There is a timing aspect I suspect, but basically because the example app had a clean task that declared some outputs of |
Technically, the problem could be seen that because we run the "Test" task as our last command in CI, it doesn't technically run format afterwards (and only keeps generated code the same due to cache hit). Perhaps moving format to run as the final task of ci would also make sense. |
Ah, good catch 👍 It's too early, but I think we can let the new script to generate esm wrappers do a bit more, so that we end up with just a single npm script for builds. One thing I have been experimenting with was a smarter clean before build, that doesn't actually delete files. That way, we don't need a clean script either, and I think this should fix the issue where the TypeScript language server fails to resolve types in the monorepo. |
@timostamm that sounds good. I keep eyeing |
## What's Changed * Remove experimental plugin by @paul-sachs in #178 * Add ESM wrapper to avoid dual package hazard by @timostamm in #180 * Fix missing implicit types by @mckelveygreg in #211 ## New Contributors * @mckelveygreg made their first contribution in #211 **Full Changelog**: v0.5.1...v0.5.2
This fixes the dual package hazard with the solution proposed in bufbuild/protobuf-es#509 (comment).
We've tested the approach in connectrpc/examples-es#1002 in various versions of Node.js, esbuild, parcel, rollup, vite, and webpack. Tree-shaking continues to work as expected with the "module" condition, and the dual package hazard is resolved with the esm wrappers in the "import" condition.
Note that we bump dependencies on
@connectrpc/connect
to^1.1.2
and@bufbuild/protobuf
to^1.3.3
with this PR, meaning that the respective version or later needs to be installed. This is necessary because users are very likely to run into a dual package hazard otherwise.