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

Wait for pending validations instead of checking on interval #53

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

Conversation

jgautier
Copy link
Contributor

@jgautier jgautier commented Jun 4, 2020

After looking through the code I thought it might be better to wait for the promises to resolve instead of checking the count on a timer. I also added a regression which I tested by checking out the 2.0.0 version of tarn.js and writing this test that passes, then I checked out the version before my other fix and see that it failed, and now it is passing on this commit and the previous fix.

@elhigu
Copy link
Collaborator

elhigu commented Jun 4, 2020

There might have been a reason why I didn't write the code like that originally... but looks like all the tests are passing so this should be good. Anyways I need to look to this more carefully, before merging.

@elhigu
Copy link
Collaborator

elhigu commented Jun 4, 2020

I might have been worried about race conditions, where some of the validations are added to the array later on because of some delay somewhere or something like that. So at least code creating those validation promises should be looked through and checked that it is not possible that more validations are added to that array if .destroy() has been called.

@kibertoad
Copy link
Contributor

@elhigu Should we rebase and merge this?

@elhigu
Copy link
Collaborator

elhigu commented Jan 17, 2021

validations are added to the array later on because of some delay somewhere or something like that

Probably the part of code that worried me is going to change to be more robust. Better not to merge this yet.

@kibertoad
Copy link
Contributor

@elhigu Where are we standing with this?

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.

3 participants