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

[Abort Controller] Update linting and fix linting errors #11269

Merged
merged 14 commits into from
Sep 21, 2020
2 changes: 1 addition & 1 deletion sdk/core/abort-controller/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
"scripts": {
"audit": "node ../../../common/scripts/rush-audit.js && rimraf node_modules package-lock.json && npm i --package-lock-only 2>&1 && npm audit",
"build:es6": "tsc -p tsconfig.json",
"build:nodebrowser": "rollup -c 2>&1",
"build:nodebrowser": "rollup -c 2>&1 && api-extractor run --local",
"build:test": "rollup -c rollup.test.config.js 2>&1",
"build": "npm run build:es6 && npm run build:nodebrowser",
"check-format": "prettier --list-different --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"test/**/*.ts\" \"*.{js,json}\"",
Expand Down
9 changes: 5 additions & 4 deletions sdk/core/abort-controller/review/abort-controller.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export class AbortController {
constructor(parentSignals?: AbortSignalLike[]);
constructor(...parentSignals: AbortSignalLike[]);
abort(): void;
readonly signal: AbortSignal;
get signal(): AbortSignal;
Copy link
Contributor

Choose a reason for hiding this comment

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

We typically downlevel TypeScript get accessor signatures to readonly properties. Was that something we were doing on the older types file that didn't get carried in?

Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize this was an accessor before, yes we should use downlevel-dts here if so

Copy link
Member Author

Choose a reason for hiding this comment

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

@willmtemple and @xirzec
So to be clear, we should not ship a get accessor at all? if so, why?
If not, would #11319 take care of this?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to avoid them since:

  1. They have TS compatibility concerns before/after 3.7
  2. They are harder for developers to reason about, as property accesses are usually assumed to be cheap and without side-effects.

There are times where for compat purposes we can't avoid them (and maybe this is one of those times) but for new code I'd like to steer away from using getters.

Copy link
Member Author

Choose a reason for hiding this comment

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

I turned off extract-api again. We already have an issue opened for it: #10320.

static timeout(ms: number): AbortSignal;
}

Expand All @@ -21,10 +21,11 @@ export class AbortError extends Error {
// @public
export class AbortSignal implements AbortSignalLike {
constructor();
readonly aborted: boolean;
get aborted(): boolean;
addEventListener(_type: "abort", listener: (this: AbortSignalLike, ev: any) => any): void;
static readonly none: AbortSignal;
onabort?: (ev?: Event) => any;
dispatchEvent(_event: Event): boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

The other four changes seem reasonable if weird, but here dispatchEvent has just appeared out of thin air. Did we modify this signature somewhere to export it?

static get none(): AbortSignal;
onabort: ((ev?: Event) => any) | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we change an undefined to null? The change in this signature doesn't seem compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also seems like the forward-declaration of Event fell out from shims-public.d.ts. API Extractor doesn't yet support rolling up file-based references, so we'll have to work around it. I think core-http has a solution of some kind, but I can't remember exactly. I think @jeremymeng knows more.

removeEventListener(_type: "abort", listener: (this: AbortSignalLike, ev: any) => any): void;
}

Expand Down