-
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
Verify api file path and enable api-extractor for abort-controller #13861
Verify api file path and enable api-extractor for abort-controller #13861
Conversation
{ | ||
Write-Host "$($packageDir) should contain only one api review for $($packageName)" | ||
Write-Host "No of Packages $($files.Count)" | ||
Write-Host "$($pkgName) does not have api review json" |
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.
What if anything should folks do if they hit this issue?
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.
Log an issue to get the api review file in.
We have #13387 that should fix this up across the repo. cc @KarishmaGhiya
We have the code generator now using the api extractor, so any newly generated package should have this.
Anybody using our template project as a reference should also get it
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 mainly asked the question here as I think we need to improve the message here to let folks know what they should do if they hit 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.
Agreed. How about
THIS IS BAD! LOG A GITHUB ISSUE FOR THIS NOW!!!!
:)
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.
Updated log message
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 addition to this change, I will check if we can update client type for util package to different type after March release. I will create and issue for tracking.There is an alternate thread on this discussion.
@praveenkuttappan You will have to commit the api review file generated with this PR and the build will pass. Run "rushx build" for this package on your local and commit the api file. |
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.
Cool pipeline feature!
onabort?: (ev?: Event) => any; | ||
dispatchEvent(_event: Event): boolean; | ||
static get none(): AbortSignal; | ||
onabort: ((ev?: Event) => any) | null; |
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.
the only minor eyebrow raise here is the switch from onabort being optional to requiring null
.
Looks like @daviwil did this 2 years ago though, so probably it's fine?
Enable API extractor for abort controller and also check API extract file path