Skip to content
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

Merged
merged 9 commits into from
Oct 6, 2023
Merged

Conversation

timostamm
Copy link
Member

@timostamm timostamm commented Sep 29, 2023

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.

@timostamm
Copy link
Member Author

@paul-sachs, do you have an idea what the CI failure is about? I just ran pnpm run all at first, expecting it to run all things. But even when repeating the steps from ci.yaml, I don't see a diff 🤔

@paul-sachs
Copy link
Collaborator

@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).

@paul-sachs
Copy link
Collaborator

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.

@paul-sachs
Copy link
Collaborator

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 dist/**, but in fact it's clean operation was just deleting the generated files (src/gen/**), this caused some confusion. Removing the clean task from the example app and just making it a part of generate fixes it.

@paul-sachs
Copy link
Collaborator

paul-sachs commented Sep 29, 2023

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.

@timostamm
Copy link
Member Author

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.

@paul-sachs
Copy link
Collaborator

@timostamm that sounds good. I keep eyeing vites lib mode, wondering if that provides a better experience over hand rolling these build processes every time but I haven't looked at how they handle the dual packages problem.

@timostamm timostamm marked this pull request as ready for review October 6, 2023 16:43
@timostamm timostamm requested review from smaye81 and paul-sachs and removed request for paul-sachs October 6, 2023 16:44
@timostamm timostamm merged commit d61b857 into main Oct 6, 2023
6 checks passed
@timostamm timostamm deleted the tstamm/esmwrapper branch October 6, 2023 17:05
@paul-sachs paul-sachs mentioned this pull request Oct 6, 2023
paul-sachs added a commit that referenced this pull request Oct 6, 2023
## 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
paul-sachs added a commit that referenced this pull request Oct 12, 2023
Due to changes made in #180, exports were referenced as `.js` files,
which means that all files must subsequently imported with the `.js`
full extension because we are declared as `type: module`.

The javascript module system just keeps on giving.

Fixes #216
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants