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

[mgmt] migrate mgmt package to esm 13 #32598

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

kazrael2119
Copy link
Contributor

@kazrael2119 kazrael2119 commented Jan 17, 2025

#32184
@@azure/arm-dnsazure/arm-dns-profile-2020-09-01-hybrid

@github-actions github-actions bot added the Mgmt This issue is related to a management-plane library. label Jan 17, 2025
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

"@vitest/coverage-istanbul": "^2.1.8",
"dotenv": "^16.0.0",
"playwright": "^1.49.1",
"tsx": "^4.7.1",
Copy link
Member

Choose a reason for hiding this comment

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

We should probably remove tsx as well. Just a note for the future.

@@ -8,11 +8,11 @@
"node": ">=18.0.0"
},
"dependencies": {
"@azure/core-lro": "^2.5.4",
"@azure/abort-controller": "^2.1.2",
Copy link
Member

Choose a reason for hiding this comment

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

We should also make sure we upgrade to @azure/abort-controller 2.1.2 if not done already. Many ARM packages are still on 1.0.0

@@ -33,7 +31,7 @@ async function createDnssecConfig() {
console.log(result);
}

async function main() {
async function main(): Promise<void> {
createDnssecConfig();
Copy link
Member

Choose a reason for hiding this comment

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

We should fix these functions because we're not awaiting the result of this so it could be an unhandled error should it throw.

Copy link
Member

@mpodwysocki mpodwysocki left a comment

Choose a reason for hiding this comment

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

Looks fine, although I have some concerns about the samples-dev not awaiting the methods they are calling, which can lead to unhandled promise rejections.

@mpodwysocki
Copy link
Member

@kazrael2119 I have a gist in which you could just do local patching using ts-morph to fix the await issues for the samples-dev as noted here: https://gist.github.com/mpodwysocki/216db5f4605cb3f972a0ae6fd45139c4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mgmt This issue is related to a management-plane library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants