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 #842

Merged
merged 10 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 @bufbuild/protobuf to ^1.3.3 with this PR, meaning that 1.3.3 or later needs to be installed. This is necessary because users are very likely to run into a dual package hazard otherwise.

@timostamm timostamm changed the title Add ESM wrapper for @connectrpc/connect Add ESM wrapper to avoid dual package hazard Sep 29, 2023
@smaye81
Copy link
Member

smaye81 commented Oct 3, 2023

For my findings re: general interop between bundlers, I tested Webpack, Parcel, Rollup, esbuild, and Vite.

For Webpack, Parcel, Rollup, and esbuild, I can get the dual package hazard to occur and can confirm installing the fix tarball fixes the problem in all 4 bundlers.

For Vite, I can't seem to get the dual package hazard to occur. I tried various incantations in my vite.config.js file, but every output builds fine and all tests for the dual package hazard pass. They also pass with the installed fix tarball, also, so it may not be an issue. Not sure if I'm missing something in the config though.

@paul-sachs
Copy link
Contributor

Investigations into tree shaking look good as well. No significant difference between bundle sizes between the current 1.0 and this version. Tried with both direct and intermediary examples, using webpack, esbuild, and parcel. Parcel was the only slight outlier in that I couldn't get tree shaking to work at all, regardless of which version I used

@timostamm
Copy link
Member Author

Sounds great, @smaye81. Can you push the tests up somewhere (examples-es) so we can re-use for Node16 module resolution and #839 as discussed?

@timostamm
Copy link
Member Author

For Vite, I can't seem to get the dual package hazard to occur. I tried various incantations in my vite.config.js file, but every output builds fine and all tests for the dual package hazard pass. They also pass with the installed fix tarball, also, so it may not be an issue. Not sure if I'm missing something in the config though.

I can reproduce the error with vite. In the branch tstamm/dual-package-hazard-example in examples-es:

Append to react/vite/src/App.tsx:

import { ConnectError as a } from "@connectrpc/connect";
import { createFetchClient as b } from "@connectrpc/connect/protocol";
import { createTransport as c } from "@connectrpc/connect/protocol-grpc";
import { createTransport as d } from "@connectrpc/connect/protocol-grpc-web";
import { createTransport as e } from "@connectrpc/connect/protocol-connect";

const direct = {
    a,
    b,
    c,
    d,
    e,
};

// @ts-ignore
import { intermediary } from "intermediary";

for (const name of "abcde".split("")) {
    // @ts-ignore
    const directSymbol = direct[name];
    const interSymbol = intermediary[name];
    if (!directSymbol) {
        throw `missing direct ${name}`;
    }
    if (!interSymbol) {
        throw `missing intermediary ${name}`;
    }
    if (directSymbol === interSymbol) {
        console.log(`${name} OK`)
    } else {
        console.error(`${name} FAIL`)
    }
}
cd react/vite
npm ci
cp -rp ../../dual-package-hazard-example/intermediary node_modules
npm start
Screenshot 2023-10-04 at 16 42 57

@smaye81
Copy link
Member

smaye81 commented Oct 4, 2023

[...]
I can reproduce the error with vite. In the branch tstamm/dual-package-hazard-example in examples-es:
[...]

Interesting. I see that too. But, when building a bundle with Vite, I can't get it to fail. I pushed up my changes to the examples-es branch. Simply run bash test.bash to see what I mean.

@timostamm
Copy link
Member Author

Investigations into tree shaking look good as well. No significant difference between bundle sizes between the current 1.0 and this version. Tried with both direct and intermediary examples, using webpack, esbuild, and parcel. Parcel was the only slight outlier in that I couldn't get tree shaking to work at all, regardless of which version I used

Thanks, @paul-sachs. Can you push the code up? It does not need to be polished at all, just repeatable.

@paul-sachs
Copy link
Contributor

paul-sachs commented Oct 4, 2023

@timostamm pushed up to your dual-package-hazard-example. Just need to run bundle command in the consumer package and inspect the resulting file sizes for all the different bundlers. I used the tree -sh --sort=size bundler-dist command.

@fubhy
Copy link
Contributor

fubhy commented Oct 5, 2023

This is great! I appreciate all the work and research that went into this. Looking forward to the next release :)

@timostamm timostamm marked this pull request as ready for review October 6, 2023 15:15
@timostamm timostamm merged commit 45e28e9 into main Oct 6, 2023
5 checks passed
@timostamm timostamm deleted the tstamm/esmwrapper branch October 6, 2023 15:20
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.

4 participants