-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
track2 @azure/arm-features #16207
track2 @azure/arm-features #16207
Conversation
/azp run prepare-pipelines |
Azure Pipelines successfully started running 1 pipeline(s). |
export type FeaturesUnregisterResponse = FeatureResult; | ||
|
||
// @public | ||
export const enum KnownSubscriptionFeatureRegistrationApprovalType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make sure all enums are just regular enums. Const enums do not work well in javascript: Azure/autorest.typescript#1013
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's an important issue. @sarangan12 could you please fix it ASAP. I think it blocks the release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created PR Azure/autorest.typescript#1097
"sourceMap": true, | ||
"declarationMap": true, | ||
"esModuleInterop": true, | ||
"allowSyntheticDefaultImports": true, | ||
"forceConsistentCasingInFileNames": true, | ||
"lib": ["es6"], | ||
"preserveConstEnums": true, | ||
"lib": ["es6", "dom"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why adding dom here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In one of the data plane SDKs, this was creating issue. I do not remember which SDK. On analysis, I found this was generic and we should add the dom also. If you feel this is not required specifically for this SDK, you can remove it if there are no issues on your side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@witemple-msft and @xirzec what do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to omit it if your SDK doesn't only work in browsers, but sometimes it's hard to avoid. I'd need to see the build error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be fine to include as long as we're careful to shim browser types that are exposed in the API surface. I took a quick look through the api.md and didn't see anything that sticks out to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is emitted by code gen (@sarangan12 for all packages, right?) so I think we're careful in the sense that code gen does not understand browser types/shims anyway.
/azp run prepare-pipelines |
Azure Pipelines successfully started running 1 pipeline(s). |
# Conflicts: # common/config/rush/pnpm-lock.yaml # rush.json
# Conflicts: # common/config/rush/pnpm-lock.yaml
No description provided.