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

Adds delay enqueue hooks #737

Closed
wants to merge 1 commit into from
Closed

Adds delay enqueue hooks #737

wants to merge 1 commit into from

Conversation

ehainer
Copy link

@ehainer ehainer commented Dec 29, 2021

This attempts to address issues raised in #721

Introduction of beforeDelayEnqueue/afterDelayEnqueue plugin hooks, which are the retry plugin equivalent of beforeEnqueue/afterEnqueue

There were still issues surrounding the frozen args object, making it impossible to manipulate arg data when entering the beforePerform hook unless workarounds like JSON.parse(JSON.stringify(this.args)) were used. To that end, the use of Object.freeze was replaced with Object.seal, and the reference to this.args from within a Plugin class was converted into a getter/setter that merges the value of reassigned args with the original args data, effectively preventing someone from adding/deleting a property from the args, but allowing them to modify properties that were in the original args. For example:

// Assume `this.args` is originally: [{ user: 'gid://User/1', flag: true, nested: { a: 1, b: 2 } }]
// ...
async beforeEnqueue(){ // Or any before/after hook
  this.args[0].flag = false // This works fine, the sealed `this.args` already contained the property `flag`
  this.args[0].newProp = "whatever" // Will throw a TypeError because `this.args` is a sealed object and `newProp` is new
  delete this.args[0].user // Will throw an error because `this.args` is a sealed object

  // Reassigning the whole args property
  this.args = [{ user: { name: 'Keanu' }, flag: false, nested: { c: 3 } }] // This will fail because the nested property `c` is new
  this.args = [{ user: { name: 'Keanu' }, flag: false, nested: { a: 88 } }] // This works, all properties are known already
}

I debated whether or not to surface the TypeError that's thrown when a sealed object is added to or removed from, but I felt that might cause confusion if you tried to assign a new property to args and it just never appeared. Having the TypeError bubble seems a bit better, so that's what it does. If you try to manipulate args in a way that's not permitted the error will be thrown and the job will fail, so you'd see that in the logs.

Sorry this took so long, @evantahler Holidays and such... Would love to get your feedback on this, I know the change to Object.freeze may seem a bit unexpected, but I'm hoping it will meet that middle ground goal of not breaking the issue raised in #99 while still permitting manipulation of args, at least within beforePerform. Perhaps I don't understand the ramifications of modifying args though.

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.

1 participant