bull-arena: support for bullmq #48886

gtpan77 · 2020-10-17T18:51:26Z

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change. (Run with npm test YOUR_PACKAGE_NAME.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: document bullmq support bee-queue/arena#248
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
Include tests for your changes
If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.

Currently needs microsoft/DefinitelyTyped-tools#137.

typescript-bot · 2020-10-20T05:42:04Z

@gtpan77 Thank you for submitting this PR!

This is a live comment which I will keep updated.

This PR doesn't modify any tests, so it's hard to know what's being fixed, and your changes might regress in the future. Have you considered adding tests to cover the change you're making? Including tests allows this PR to be merged by yourself and the owners of this module. This can potentially save days of time for you.

1 package in this PR

bull-arena on npm, on unpkg

Code Reviews

This PR can be merged once it's reviewed.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ Most recent commit is approved by type definition owners or DT maintainers

All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 48886,
  "author": "gtpan77",
  "owners": [
    "levibostian",
    "gtpan77"
  ],
  "dangerLevel": "ScopedAndUntested",
  "headCommitAbbrOid": "0dc2422",
  "headCommitOid": "0dc24227f8f526504c19a60fda1223a2ed9af9df",
  "mergeIsRequested": true,
  "stalenessInDays": 0,
  "lastPushDate": "2020-10-20T14:28:06.000Z",
  "reopenedDate": "2020-10-20T05:41:23.000Z",
  "lastCommentDate": "2020-10-20T17:42:54.000Z",
  "maintainerBlessed": true,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/48886/files",
  "hasMergeConflict": false,
  "authorIsOwner": true,
  "isFirstContribution": false,
  "popularityLevel": "Well-liked by everyone",
  "newPackages": [],
  "packages": [
    "bull-arena"
  ],
  "files": [
    {
      "path": "types/bull-arena/index.d.ts",
      "kind": "definition",
      "package": "bull-arena"
    },
    {
      "path": "types/bull-arena/package.json",
      "kind": "package-meta-ok",
      "package": "bull-arena"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-10-20T16:40:00.000Z",
  "firstApprovalDate": "2020-10-20T16:40:00.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 2,
  "isChangesRequested": false
}

typescript-bot · 2020-10-20T05:42:05Z

🔔 @levibostian — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

typescript-bot · 2020-10-20T05:42:06Z

@gtpan77 The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot · 2020-10-20T08:11:13Z

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

Comparison details 📊

	master	#48886	diff
Batch compilation
Memory usage (MiB)	84.1	84.2	+0.1%
Type count	11503	11860	+3%
Assignability cache size	2153	2259	+5%

Language service
Samples taken	17	17	0%
Identifiers in tests	17	17	0%
`getCompletionsAtPosition`
Mean duration (ms)	527.3	524.3	-0.6%
Mean CV	10.7%	9.3%
Worst duration (ms)	767.7	782.5	+1.9%
Worst identifier	arena	arena
`getQuickInfoAtPosition`
Mean duration (ms)	517.7	518.3	+0.1%
Mean CV	11.4%	10.5%	-7.5%
Worst duration (ms)	739.5	666.6	-9.9%
Worst identifier	use	use

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

levibostian · 2020-10-20T14:04:58Z

types/bull-arena/index.d.ts

@@ -3,12 +3,13 @@
 // Definitions by: Levi Bostian <https://github.com/levibostian>
 //                 Gaurav Sharma <https://github.com/gtpan77>
 // Definitions: https://github.com/DefinitelyTyped/DefinitelyTyped
-// TypeScript Version: 2.8
+// TypeScript Version: 4.0


What is the reason for bumping this up?

will revert

Build is failing when I reverted to 2.8

Accessors are only available when targeting ECMAScript 5 and higher.

I am not knowledgeable on that error. But, from looking at this PR, I don't see a need to bump the version as the change is quite minimal. That's why I asked about the bump.

CI build was failing on 2.8. When I changed it to 4.0, build passed without any problem. I guess bullmq is using latest version of typescript. I am not familiar with Accessors.

levibostian · 2020-10-20T14:06:50Z

types/bull-arena/index.d.ts

@@ -33,7 +35,7 @@ declare namespace BullArena {
    interface QueueOptions {
        name: string;
        hostId?: string;
-        type?: "bull" | "bee";
+        type?: "bull" | "bee" | string;


I am not sure how Arena deals with bullmq. Do we send "bullmq" as the type param when using bullmq? If we can make the type stronger then string here that would be better.

Thanks for pointing this. I'll add bullmq but there is a reason for adding string because of a compilation error.
string is in use in prefix param. Similarly I used it here and it fixed the issue.

Type '{ name: string; hostId: string; type: string; url: string; }[]' is not assignable to type '((QueueOptions & PortHostConnectionOptions) | (QueueOptions & RedisUrlConnectionOptions) | (QueueOptions & RedisClientConnectionOptions))[]'. Type '{ name: string; hostId: string; type: string; url: string; }' is not assignable to type '(QueueOptions & PortHostConnectionOptions) | (QueueOptions & RedisUrlConnectionOptions) | (QueueOptions & RedisClientConnectionOptions)'. Type '{ name: string; hostId: string; type: string; url: string; }' is not assignable to type 'QueueOptions & RedisUrlConnectionOptions'. Type '{ name: string; hostId: string; type: string; url: string; }' is not assignable to type 'QueueOptions'. Types of property 'type' are incompatible. Type 'string' is not assignable to type '"bull" | "bee" | undefined'.

I don't understand the compilation error. Does the compiler pass before your changes? Before your PR, | string didn't exist. That's why I don't think we need it now.

Whenever I pass type parameter in Arena constructor, then this compilation error pops up.

Could you add some tests to the definitions that show Arena construction?

This compilation error was coming even before my changes. This error is not related to these changes.

I have tested it by making a sample project. See prefix parameter in Arena constructor where it is allowed to have any string.

typescript-bot · 2020-10-20T14:07:31Z

@gtpan77 One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you!

typescript-bot · 2020-10-20T14:24:12Z

@gtpan77 The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

typescript-bot · 2020-10-20T14:32:47Z

@levibostian Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

gtpan77 · 2020-10-20T17:42:54Z

Ready to merge

typescript-bot · 2020-10-20T18:07:43Z

I just published @types/[email protected] to npm.

skeggse · 2020-10-21T18:08:43Z

I would suggest not shipping new type definitions until the underlying feature has landed in the underlying library - bee-queue/arena#251 hasn't landed yet.

gtpan77 · 2020-10-21T18:47:28Z

These changes will not affect the existing code. I'll submit requested changes of PR 251 before weekend.

support for bullmq

35b1afa

gtpan77 mentioned this pull request Oct 17, 2020

feat: allow bullmq for bull-arena types microsoft/DefinitelyTyped-tools#137

Merged

gtpan77 marked this pull request as ready for review October 20, 2020 05:41

typescript-bot added The CI failed When GH Actions fails Author is Owner The author of this PR is a listed owner of the package. Untested Change This PR does not touch tests labels Oct 20, 2020

re-run CI

fee32ac

typescript-bot removed the The CI failed When GH Actions fails label Oct 20, 2020

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Oct 20, 2020

levibostian suggested changes Oct 20, 2020

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Oct 20, 2020

revert ts version and bullmq type param

1a6b009

typescript-bot added The CI failed When GH Actions fails and removed Revision needed This PR needs code changes before it can be merged. labels Oct 20, 2020

ts 4.0

0dc2422

typescript-bot removed the The CI failed When GH Actions fails label Oct 20, 2020

levibostian approved these changes Oct 20, 2020

View reviewed changes

typescript-bot added the Owner Approved A listed owner of this package signed off on the pull request. label Oct 20, 2020

typescript-bot added the Self Merge This PR can now be self-merged by the PR author or an owner label Oct 20, 2020

typescript-bot merged commit a0ddf5c into DefinitelyTyped:master Oct 20, 2020

gtpan77 mentioned this pull request Oct 20, 2020

add support for bullmq bee-queue/arena#251

Merged

skeggse mentioned this pull request Nov 1, 2020

document bullmq support bee-queue/arena#248

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bull-arena: support for bullmq #48886

bull-arena: support for bullmq #48886

gtpan77 commented Oct 17, 2020

typescript-bot commented Oct 20, 2020 •

edited

Loading

typescript-bot commented Oct 20, 2020

typescript-bot commented Oct 20, 2020

typescript-bot commented Oct 20, 2020

levibostian Oct 20, 2020

gtpan77 Oct 20, 2020

gtpan77 Oct 20, 2020

levibostian Oct 20, 2020

gtpan77 Oct 20, 2020

levibostian Oct 20, 2020

gtpan77 Oct 20, 2020

levibostian Oct 20, 2020

gtpan77 Oct 20, 2020

levibostian Oct 20, 2020

gtpan77 Oct 20, 2020

gtpan77 Oct 20, 2020

typescript-bot commented Oct 20, 2020

typescript-bot commented Oct 20, 2020

typescript-bot commented Oct 20, 2020

gtpan77 commented Oct 20, 2020

typescript-bot commented Oct 20, 2020

skeggse commented Oct 21, 2020

gtpan77 commented Oct 21, 2020 •

edited

Loading

bull-arena: support for bullmq #48886

bull-arena: support for bullmq #48886

Conversation

gtpan77 commented Oct 17, 2020

typescript-bot commented Oct 20, 2020 • edited Loading

1 package in this PR

Code Reviews

Status

typescript-bot commented Oct 20, 2020

typescript-bot commented Oct 20, 2020

typescript-bot commented Oct 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typescript-bot commented Oct 20, 2020

typescript-bot commented Oct 20, 2020

typescript-bot commented Oct 20, 2020

gtpan77 commented Oct 20, 2020

typescript-bot commented Oct 20, 2020

skeggse commented Oct 21, 2020

gtpan77 commented Oct 21, 2020 • edited Loading

typescript-bot commented Oct 20, 2020 •

edited

Loading

gtpan77 commented Oct 21, 2020 •

edited

Loading