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

Fix the bug that commonjs doesn't work for @azure/web-pubsub-client-protobuf #27209

Merged
merged 3 commits into from
Sep 26, 2023

Conversation

zackliu
Copy link
Member

@zackliu zackliu commented Sep 22, 2023

Packages impacted by this PR

@azure/web-pubsub-client-protobuf

Issues associated with this PR

#25703

Describe the problem that is addressed by this PR

As protobufjs is purely commonjs type but rollup bundle didn't add commonjs support. So only file in dist-esm/ works but in dist doesn't.

Are there test cases added in this PR? (If not, why?)

Test already added cjs() plugin in bundle, so it works.

Provide a list of related PRs (if any)

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

  • Added impacted package name to the issue description
  • Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
  • Added a changelog (if necessary)

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@jeremymeng
Copy link
Member

I believe dev-tool run bundle command generates commonjs to dist/index.js. Is it missing anything that your need? I see that you have exports: "named", which is not in our base config. /cc @willmtemple

plugins: [sourcemaps(), nodeResolve(), cjs(), json()],

@zackliu
Copy link
Member Author

zackliu commented Sep 22, 2023

@jeremymeng Maybe I'm wrong, but I think dev-tool run bundle uses this config. Add tried to add cmj() to it locally to have a test and it works.

plugins: [nodeResolve(), sourcemaps()],

@jeremymeng
Copy link
Member

@zackliu you are right! I don't know why we don't have cjs() there. Could be a bug. @willmtemple do you remember?

@@ -0,0 +1,33 @@
import nodeBuiltins from "builtin-modules";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use makeConfig from dev-tool if it works for you?

import { makeConfig } from "@azure/dev-tool/shared-config/rollup";

export makeConfig(require("./package.json"));

@jeremymeng
Copy link
Member

I don't know why we don't have cjs() there.

maybe because the assumption is that all input are ESM. But in your case, if I understand correctly, some generated CJS files got copied over?

@zackliu
Copy link
Member Author

zackliu commented Sep 25, 2023

@jeremymeng I tried, and it works. Yes in this case, protobufjs is cjs and needs to be copied in dist

@jeremymeng
Copy link
Member

builds failing likely because of missing rush update

@zackliu
Copy link
Member Author

zackliu commented Sep 26, 2023

@jeremymeng Test passed. Please help to merge it.

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

Successfully merging this pull request may close these issues.

4 participants