-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add methods to bull #151
Add methods to bull #151
Conversation
🦋 Changeset detectedLatest commit: 4e01909 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request introduces several modifications across different packages. In the Bull package, a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@zemble/auth-anonymous
@zemble/auth
@zemble/auth-api-token
@zemble/auth-apple
@zemble/auth-otp
@zemble/bull
@zemble/bun
@zemble/core
create-zemble-app
create-zemble-plugin
@zemble/debug
@zemble/email-resend
@zemble/email-sendgrid
eslint-config-kingstinct
@zemble/firebase-auth
@zemble/graphql
@zemble/kv
@zemble/migrations
@zemble/mongodb
@zemble/node
@zemble/pino
@zemble/push-apple
@zemble/push-expo
@zemble/routes
@zemble/react
@zemble/sms-46elks
@zemble/sms-twilio
@zemble/supabase
@zemble/utils
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.changeset/dry-owls-applaud.md (1)
1-5
: Consider adding more detail to the changeset description.While the current description is accurate, it could be more informative for future reference.
Consider expanding the description:
--- "@zemble/bull": patch --- -added getDelayed to ZembleQueueMock +added getDelayed method to ZembleQueueMock to support querying delayed jobs in test environments
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/dry-owls-applaud.md
(1 hunks)packages/bull/ZembleQueueMock.ts
(3 hunks)packages/push-apple/graphql/schema.generated.ts
(1 hunks)packages/push-expo/graphql/schema.generated.ts
(0 hunks)
💤 Files with no reviewable changes (1)
- packages/push-expo/graphql/schema.generated.ts
🔇 Additional comments (4)
packages/bull/ZembleQueueMock.ts (2)
20-20
: LGTM! Interface update is well-typed.The
getDelayed
method is correctly added to the interface with proper type inheritance fromZembleQueueBull
.
42-42
: LGTM! Constructor binding follows established pattern.The
getDelayed
method is properly bound withjest.fn()
for test spying capabilities, consistent with other method bindings.packages/push-apple/graphql/schema.generated.ts (2)
Line range hint
1-235
: Verify if committing this generated file is intentionalThis appears to be an auto-generated GraphQL schema file (
.generated.ts
). A few concerns:
- Generated files are typically not committed unless there's a specific reason
- The changes in this file (Apple Push notifications) don't align with the PR objectives of "Add methods to bull"
Please verify if:
- These changes should be part of this PR
- The source GraphQL schema files should be committed instead
✅ Verification successful
Generated schema files are part of the project convention
The presence of
.generated.ts
files across all packages and corresponding codegen configurations indicates that committing generated GraphQL schema files is an intentional project convention. This file should remain in the PR.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are corresponding source GraphQL schema files fd -e graphql -e gqlLength of output: 1381
Script:
#!/bin/bash # Check for codegen configuration files fd 'codegen|graphql.*config' -t f # Check if there are other .generated.ts files fd '\.generated\.ts$' # Check if the source schema files are modified in this PR git diff HEAD~1 packages/push-apple/graphql/schema.graphql packages/push-apple/graphql/schema.local.graphqlLength of output: 2304
219-219
: Breaking Change: Verify impact of removing liveActivityAttributesThe removal of the
liveActivityAttributes
field fromregisterAppleUpdateLiveActivityPushToken
is a breaking change that could affect existing clients. Please ensure that:
- This change is intentional and documented
- All clients are updated to handle this change
- The version number is bumped appropriately (semver major)
// eslint-disable-next-line class-methods-use-this | ||
async getDelayed() { | ||
return [] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider enhancing the mock implementation.
The current implementation has several areas for improvement:
- The empty array return value might not accurately represent delayed jobs in the queue
- The
eslint-disable
comment seems unnecessary for an async method - Missing documentation to explain the implementation choice
Consider this enhanced implementation:
- // eslint-disable-next-line class-methods-use-this
- async getDelayed() {
- return []
- }
+ /**
+ * Gets delayed jobs from the mock queue.
+ * @returns Array of delayed jobs based on JobsOptions.delay
+ */
+ async getDelayed() {
+ return this.jobs.filter(job =>
+ job.opts?.delay !== undefined &&
+ job.opts.delay > 0
+ )
+ }
This implementation would:
- Return actual delayed jobs based on their options
- Remove unnecessary eslint comment
- Add documentation
- Better mirror the real Bull queue behavior
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Summary by CodeRabbit
New Features
getDelayed
method toZembleQueueMock
for retrieving delayed queue itemsRefactor
JSONObject
scalar type from Apple push notifications GraphQL schemaQuery
andSubscription
typesBug Fixes