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

Convert to Typescript #585

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

BehindTheMath
Copy link
Contributor

@BehindTheMath BehindTheMath commented Mar 1, 2021

Convert code and tests to Typescript.

There are still several issues:

  • The bundled type for cron-parser are incorrect, and not everything is exported. I submitted Fix types so that all types are exported harrisiirak/cron-parser#210 to fix the types, but I realized that PR is wrong as well and needs to be corrected. In the meantime I bundled the accurate type declarations for cron-parser as cron-parser.d.ts.
  • prepareNextInvocation() in Invocation.ts uses CronExpression._endDate which is an undocumented and unexported property. I included it in the bundled type declarations for cron-parser, but I don't think this is an optimal solution.
  • convenience-method-tests.ts uses invocations and job.pendingInvocations. These should really be private and unexported, and should be tested differently.
  • I changed the npm script for test to use ts-node to run tape. I don't have any experience with airtap, so I didn't convert the test:browser script. In addition, I couldn't figure out how to convert the nyc scripts.
  • I did not add any TS-specific ESLint rule.
  • Job supports passing a generator function that returns a iterator that returns the job function (see Add support for generators #210). I think this use case is extremely uncommon nowadays. It looks like the original PR was designed to be able to support asynchronous functions that used generators, whereas now everyone would use async functions or Promises. I think support for generators should be dropped and replaced with support for Promises / async functions.
  • I think support for Node v6 and v8 should be dropped, since those reached EOL a while ago.

Closes #507.

@kibertoad
Copy link
Contributor

@BehindTheMath Are you interested in finishing this? I have some time now and can contribute.

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

Successfully merging this pull request may close these issues.

@types/node-schedule not update for tz
2 participants