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

[Batch] Fix missing SharedKeyCredentials export #19736

Merged
merged 3 commits into from
Jan 10, 2022
Merged

Conversation

dpwatrous
Copy link
Member

@dpwatrous dpwatrous commented Jan 7, 2022

Rollup was using batchServiceClient.js as an entry point, which did not include the SharedKeyCredentials class. (See issue #18328). In the course of fixing that, changed some rollup configuration due to SharedKeyCredentials relying on commonjs modules and the built-in node.js buffer module.

@ghost ghost added the Batch label Jan 7, 2022
@dpwatrous dpwatrous force-pushed the dpwatrous/rollup-fix branch from a8f8771 to 2564a93 Compare January 7, 2022 20:37
@dpwatrous dpwatrous force-pushed the dpwatrous/rollup-fix branch from a791617 to e37d477 Compare January 7, 2022 21:06
@dpwatrous dpwatrous force-pushed the dpwatrous/rollup-fix branch from e37d477 to bf69405 Compare January 7, 2022 21:08
@dpwatrous dpwatrous force-pushed the dpwatrous/rollup-fix branch from bf69405 to 31a3446 Compare January 7, 2022 21:17
@dpwatrous dpwatrous requested a review from deyaaeldeen January 7, 2022 21:51
@dpwatrous dpwatrous marked this pull request as ready for review January 7, 2022 21:51
@dpwatrous
Copy link
Member Author

@deyaaeldeen just to give you some context, we were trying to update the Batch samples to the latest 10.0.2 SDK version and noticed that SharedKeyCredentials weren't in the rollup output. This PR should fix that, although it was a little more complex than I had originally thought due to the node.js-specific dependencies of SharedKeyCredentials

Copy link
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

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

Looks great!

I actually had to do some work for exposing this class before in #18469 but I forgot to update the entry point to point to the new index module.

@dpwatrous
Copy link
Member Author

Gotcha - I saw your other PR and figured it was something like that. Thanks!

@dpwatrous dpwatrous merged commit 82bc0dc into main Jan 10, 2022
@dpwatrous dpwatrous deleted the dpwatrous/rollup-fix branch January 10, 2022 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants