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

fix for variable name and add typescript typings #4

Merged
merged 4 commits into from Jan 13, 2020
Merged

fix for variable name and add typescript typings #4

merged 4 commits into from Jan 13, 2020

Conversation

retorquere
Copy link
Contributor

this PR fixes a variable that accidentally was replaced by its type, adds typescript typings, and updates npm modules. The existing modules didn't pass npm audit, they currently do even if that generates warnings when you run the tests; the warnings relate to this and given that discussion it seems to me it's better to have the updates with warnings than the old modules.

@coveralls
Copy link

coveralls commented Jan 11, 2020

Pull Request Test Coverage Report for Build 25

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 21: 0.0%
Covered Lines: 54
Relevant Lines: 54

💛 - Coveralls

@cmseaton42
Copy link
Owner

First of all, thanks for the PR! Ive been wanting to go back and add typings for this module for some time now.

That said, should be make the types more specific (vs using any all around)?

@cmseaton42 cmseaton42 added the enhancement New feature or request label Jan 11, 2020
@retorquere
Copy link
Contributor Author

compare_func could be more specific, but for the others, the user can pass anything, no?

@cmseaton42
Copy link
Owner

cmseaton42 commented Jan 12, 2020

What about something like this?

declare type Arguments<T> = [T] extends [(...args: infer U) => any] ? U : [T] extends [void] ? [] : [T];

declare type Task<T> = () => Promise<T>;

declare class TaskEasy<C> {
    constructor(compare_func: (a: C, b: C) => boolean, max_queue_size?: number);
    schedule<P, T extends Task<P>>(task: T, args: Arguments<T>, priority_obj: C): Promise<P>;
}

I got the Arguments type from another project.

This is totally untested by the way. Just threw something together. Thoughts?

@retorquere
Copy link
Contributor Author

I am not that well versed in typescript :)

@cmseaton42
Copy link
Owner

no worries, I'll try and test it soon and see if it works out. If so, ill send you the updates for your PR and then pull I will pull it once all is well. Thanks again 😄

@cmseaton42
Copy link
Owner

I went ahead a got a version of what i had above working (see below)...

// Per Module-class.d.ts documentation
export = TaskEasy;

// Task Easy Class
declare class TaskEasy<C> {
    constructor(compare_func: (ob1: C, obj2: C) => boolean, max_queue_size?: number);
    schedule<P, T extends TaskEasy.Task<P>>(task: T, args: TaskEasy.Arguments<T>, priority_obj: C): Promise<P>;
}

declare namespace TaskEasy {
    // Extract argument types from passed function type
    export type Arguments<T> = [T] extends [(...args: infer U) => any] ? U : [T] extends [void] ? [] : [T];

    // Generic task type, must return promise
    export type Task<T> = (...args: any[]) => Promise<T>;
}

@cmseaton42
Copy link
Owner

The typescript usage of the example from the the README becomes as follows...

import TaskEasy from "task-easy";

// Define interface for priority
//  objects to be used in the
//  TaskEasy instance
interface IPriority {
    priority: number;
    timestamp: Date;
}

// Define delay function type
// -> Must extend  Task<T>: (...args) => Promise<T>
type delayFn = (ms: number) => Promise<undefined>;

// Define delay function of type 'delayFn' defined above
const delay: delayFn = ms => new Promise(resolve => setTimeout(resolve, ms));

// Define priority function
// -> Must extend (obj1: T, obj2: T) =>
const prioritize = (obj1: IPriority, obj2: IPriority) => {
    return obj1.priority === obj2.priority
        ? obj1.timestamp.getTime() < obj2.timestamp.getTime() // Return true if task 1 is older than task 2
        : obj1.priority > obj2.priority; // return true if task 1 is higher priority than task 2
};

// Initialize new queue
const queue = new TaskEasy(prioritize); // equivalent of TaskEasy<IPriority>(prioritize) via type inference

// .schedule accepts the task signature,
// an array or arguments, and a priority object
// -> with type inference
const task1 = queue
    .schedule(delay, [100], { priority: 1, timestamp: new Date() })
    .then(() => console.log("Task 1 ran..."));

const task2 = queue
    .schedule(delay, [100], { priority: 1, timestamp: new Date() })
    .then(() => console.log("Task 2 ran..."));

// Definitely typed
const task3 = queue
    .schedule<undefined, delayFn>(delay, [100], { priority: 2, timestamp: new Date() })
    .then(() => console.log("Task 3 ran..."));

const task4 = queue
    .schedule<undefined, delayFn>(delay, [100], { priority: 1, timestamp: new Date() })
    .then(() => console.log("Task 4 ran..."));

const task5 = queue
    .schedule<undefined, delayFn>(delay, [100], { priority: 3, timestamp: new Date() })
    .then(() => console.log("Task 5 ran..."));

// OUTPUT
// Task 1 ran...
// Task 5 ran...
// Task 3 ran...
// Task 2 ran...
// Task 4 ran...

@cmseaton42 cmseaton42 merged commit 0893299 into cmseaton42:master Jan 13, 2020
@cmseaton42
Copy link
Owner

cmseaton42 commented Jan 13, 2020

@retorquere You should be able to update to v0.2.1 to get the types. Let me know if you find any bugs 🐛. I tested locally and it seems to be working as intended.

@retorquere
Copy link
Contributor Author

When I have

new TaskEasy((t1, t2) => t1.priority === t2.priority ? t1.timestamp.getTime() < t2.timestamp.getTime() : t1.priority > t2.priority)

I get

TS2339: Property 'priority' does not exist on type 'unknown'.

@retorquere
Copy link
Contributor Author

Oh wait sorry, have to declare the prio type. Yep, that works!

@retorquere
Copy link
Contributor Author

I do have to make the import

import TaskEasy = require("task-easy");

Otherwise I must enable esModuleInterop and that gave me problems in the past in my builds.

@cmseaton42
Copy link
Owner

Can you not import it with import TaskEasy from 'task-easy';??? This import worked for me when I was testing locally.

@retorquere
Copy link
Contributor Author

retorquere commented Jan 13, 2020

If I do

import TaskEasy from 'task-easy'

I get

TS1259: Module '"node_modules/task-easy/src/index"' can only be default-imported using the 'esModuleInterop' flag

And turning on the esModuleInterop gave me a lot of trouble last time. The behavior for = require is described here; from what I gather the preferred format for exports is

module.exports = {
  TaskEasy
}

and then I can do import { TaskEasy } from 'task-easy'. import = require works too and it's supported in typescript, but IIRC treeshaking won't work.

@cmseaton42
Copy link
Owner

I see... I will update the module to export in the way that you have alluded to.

module.exports = {
  TaskEasy
}

Since this is technically a breaking change, I will release a new major revision v1.0.0

@retorquere
Copy link
Contributor Author

Hold on, let me run my tests on that

@retorquere
Copy link
Contributor Author

The declaration file now looks like

// Per Module-class.d.ts documentation
  
// Task Easy Class
export class TaskEasy<C> {
    constructor(compare_func: (ob1: C, obj2: C) => boolean, max_queue_size?: number);
    schedule<P, T extends TaskEasy.Task<P>>(task: T, args: TaskEasy.Arguments<T>, priority_obj: C): Promise<P>;
}

declare namespace TaskEasy {
    // Extract argument types from passed function type
    export type Arguments<T> = [T] extends [(...args: infer U) => any] ? U : [T] extends [void] ? [] : [T];

    // Generic task type, must return promise
    export type Task<T> = (...args: any[]) => Promise<T>;
}

for me and indeed the module.exports as above.

@cmseaton42
Copy link
Owner

The module should be updated now 🎉

Let me know if all is well :)

@retorquere
Copy link
Contributor Author

All is well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants