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

Add promises-aplus tests for the promise library #159

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

forivall
Copy link

I was curious if the promise implementation you have is Promises/A+ conformant, and unfortunately, it's not, since it appears to be more of a jquery promise-compatible, which does some things synchronously that should be async.

The results summary:

266 passing (2m)
592 failing

@josdejong
Copy link
Owner

Thanks for your inputs Emily. The reason we couldn't use a standard Promise library (or nowadays the Promise implementation built in JS itself) is that we need support for cancellation. So I think it will not be possible to get the workerpool version of Promise standards compliant, though we could set some steps in that direction I guess.

What we could do is rename the (internal) Promise class to CancellablePromise or something like that to make it more clear that it's not a standard Promise implementation.

What do you think?

@forivall
Copy link
Author

Yup, i totally understand the need for cancellation.

Renaming makes sense, and another option would be to extend the native promise type, such as in https://github.com/sindresorhus/p-cancelable . Although, I also don't know if you need the tight control over synchronicity from your custom promise implementation.

@josdejong
Copy link
Owner

It would be nice to see if we can replace Promise implementation with an external library like p-cancelable. I'm not sure if there would be incompatibilities and if it's possible to achieve or not, but it may be worth an experiment. Anyone interested in trying this out?

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.

2 participants