Skip to content

fix: prevent double reflection log #1210

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

Merged
merged 3 commits into from
Jul 10, 2020
Merged

fix: prevent double reflection log #1210

merged 3 commits into from
Jul 10, 2020

Conversation

Weakky
Copy link
Collaborator

@Weakky Weakky commented Jul 10, 2020

Closes #1212

@Weakky Weakky requested a review from jasonkuhrt July 10, 2020 17:33
return (async (...args: any[]) => {
if (executing) {
// if there's already an execution, make it pending
pendingExecution = args
return null as any
return { type: 'executing' }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be simpler to return a deferred promise here? Then the caller doesn't need to know about the inner workings between result/executing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's a deferred promise in this case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return a promise here, resolve it somewhere else later. (e.g. not within the promise func body)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That won't work because promises are eager and we don't want that promise to run at all, until the one that is already running is finished. Otherwise we're not debouncing anything. We're just running the functions one after the other.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, that wouldn't help, because you need to dedupe higher up in the call stack.

Comment on lines 361 to 364
if (pendingExecution) {
await fn(...args).catch((e) => console.error(e))
res = await fn(...args).catch((e) => console.error(e))
pendingExecution = null
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do prefer it, hence why we run it right after the current promise is finished

Copy link
Member

@jasonkuhrt jasonkuhrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Weakky Weakky merged commit 36863e2 into master Jul 10, 2020
@Weakky Weakky deleted the fix/double-reflection-log branch July 10, 2020 17:55
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.

Reflection is sometimes printing errors twice
2 participants