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

Upgrade TypeScript #102

Merged
merged 3 commits into from
Mar 29, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
test/
bench.ts
8 changes: 4 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,20 +48,20 @@
"@sindresorhus/tsconfig": "^0.6.0",
"@types/benchmark": "^1.0.31",
"@types/node": "^12.12.7",
"@typescript-eslint/eslint-plugin": "^1.11.0",
"@typescript-eslint/parser": "^1.11.0",
"@typescript-eslint/eslint-plugin": "^2.25.0",
"@typescript-eslint/parser": "^2.25.0",
"ava": "^2.0.0",
"benchmark": "^2.1.4",
"codecov": "^3.3.0",
"del-cli": "^3.0.0",
"delay": "^4.2.0",
"eslint-config-xo-typescript": "^0.16.0",
"eslint-config-xo-typescript": "^0.27.0",
"in-range": "^2.0.0",
"nyc": "^14.0.0",
"random-int": "^2.0.0",
"time-span": "^3.1.0",
"ts-node": "^8.3.0",
"typescript": "3.7.2",
"typescript": "^3.8.3",
bobjflong marked this conversation as resolved.
Show resolved Hide resolved
"xo": "^0.25.3"
},
"ava": {
Expand Down
22 changes: 10 additions & 12 deletions source/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ type Task<TaskResultType> =
| (() => PromiseLike<TaskResultType>)
| (() => TaskResultType);

const empty = (): void => {};
const empty = (): void => { /* do nothing */ };
bobjflong marked this conversation as resolved.
Show resolved Hide resolved

const timeoutError = new TimeoutError();

Expand Down Expand Up @@ -53,27 +53,25 @@ export default class PQueue<QueueType extends Queue<RunFunction, EnqueueOptionsT

private readonly _throwOnTimeout: boolean;

constructor(options?: Options<QueueType, EnqueueOptionsType>) {
constructor(passedOptions?: Options<QueueType, EnqueueOptionsType>) {
super();

// eslint-disable-next-line @typescript-eslint/no-object-literal-type-assertion
options = {
const options: Options<QueueType, EnqueueOptionsType> = {
carryoverConcurrencyCount: false,
intervalCap: Infinity,
interval: 0,
concurrency: Infinity,
autoStart: true,
queueClass: PriorityQueue,
...options
// TODO: Remove this `as`.
} as Options<QueueType, EnqueueOptionsType>;
...passedOptions
} as any;
bobjflong marked this conversation as resolved.
Show resolved Hide resolved

if (!(typeof options.intervalCap === 'number' && options.intervalCap >= 1)) {
throw new TypeError(`Expected \`intervalCap\` to be a number from 1 and up, got \`${options.intervalCap}\` (${typeof options.intervalCap})`);
throw new TypeError(`Expected \`intervalCap\` to be a number from 1 and up, got \`${options.intervalCap?.toString() ?? ''}\` (${typeof options.intervalCap})`);
}

if (options.interval === undefined || !(Number.isFinite(options.interval) && options.interval >= 0)) {
throw new TypeError(`Expected \`interval\` to be a finite number >= 0, got \`${options.interval}\` (${typeof options.interval})`);
throw new TypeError(`Expected \`interval\` to be a finite number >= 0, got \`${options.interval?.toString() ?? ''}\` (${typeof options.interval})`);
}

this._carryoverConcurrencyCount = options.carryoverConcurrencyCount!;
Expand Down Expand Up @@ -205,8 +203,7 @@ export default class PQueue<QueueType extends Queue<RunFunction, EnqueueOptionsT
Executes all queued functions until it reaches the limit.
*/
private _processQueue(): void {
// eslint-disable-next-line no-empty
while (this._tryToStartAnother()) {}
while (this._tryToStartAnother()) { /* do nothing */ }
}

get concurrency(): number {
Expand All @@ -226,6 +223,7 @@ export default class PQueue<QueueType extends Queue<RunFunction, EnqueueOptionsT
/**
Adds a sync or async task to the queue. Always returns a promise.
*/
// eslint-disable-next-line @typescript-eslint/prefer-readonly-parameter-types
async add<TaskResultType>(fn: Task<TaskResultType>, options: Partial<EnqueueOptionsType> = {}): Promise<TaskResultType> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's tricky to use Readonly types here because the {} assignment.

Copy link
Owner

Choose a reason for hiding this comment

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

How so? Can you elaborate? If it's not possible to use readonly here, it sounds like a the linter rule should be adjusted to ignore this case.

Copy link
Contributor Author

@bobjflong bobjflong Mar 27, 2020

Choose a reason for hiding this comment

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

If you type options as

Readonly<Partial<EnqueueOptionsType>> = {}

Then this type error is returned:

Type '{}' is not assignable to type 'Readonly<Partial<EnqueueOptionsType>>'.ts(2322)

And the linter does want options to be Readonly afaict:

source/index.ts:229:58
  ✖  229:58  Parameter should be a read only type

Not an expert, but this indeed could be a linting issue I think. We shouldn't require options to be readonly in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know what you'd prefer in this case. Happy to look into that linting thing too if it's confirmed an issue.

Copy link
Owner

Choose a reason for hiding this comment

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

If TypeScript doesn't allow it, it's a linter bug. Can you open an issue here: https://github.com/typescript-eslint/typescript-eslint ?

return new Promise<TaskResultType>((resolve, reject) => {
const run = async (): Promise<void> => {
Expand Down Expand Up @@ -349,7 +347,7 @@ export default class PQueue<QueueType extends Queue<RunFunction, EnqueueOptionsT

For example, this can be used to find the number of items remaining in the queue with a specific priority level.
*/
sizeBy(options: Partial<EnqueueOptionsType>): number {
sizeBy(options: Readonly<Partial<EnqueueOptionsType>>): number {
return this._queue.filter(options).length;
}

Expand Down
13 changes: 9 additions & 4 deletions source/priority-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,22 @@ export default class PriorityQueue implements Queue<RunFunction, PriorityQueueOp
return;
}

const index = lowerBound(this._queue, element, (a, b) => b.priority! - a.priority!);
const index = lowerBound(
this._queue, element,
(a: Readonly<PriorityQueueOptions>, b: Readonly<PriorityQueueOptions>) => b.priority! - a.priority!
);
this._queue.splice(index, 0, element);
}

dequeue(): RunFunction | undefined {
const item = this._queue.shift();
return item && item.run;
return item?.run;
}

filter(options: Partial<PriorityQueueOptions>): RunFunction[] {
return this._queue.filter(element => element.priority === options.priority).map(element => element.run);
filter(options: Readonly<Partial<PriorityQueueOptions>>): RunFunction[] {
return this._queue.filter(
(element: Readonly<PriorityQueueOptions>) => element.priority === options.priority
).map((element: Readonly<{ run: RunFunction }>) => element.run);
}

get size(): number {
Expand Down