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

Make use of JS Promises #28

Closed
wants to merge 2 commits into from
Closed

Make use of JS Promises #28

wants to merge 2 commits into from

Conversation

Max13
Copy link

@Max13 Max13 commented Aug 16, 2016

At first: The least invasive changes. No indentation to make the "commit" readable.
Secondly: Indentation cleanup.

Also, redondants tests are removed.

Max13 added 2 commits August 17, 2016 01:22
- Least invasive changes at first
- Redundant tests were removed
@dchest
Copy link
Owner

dchest commented Aug 17, 2016

Sorry, this package was intended to be compatible even with the old IE versions, which is why it only uses typed arrays if it can detect their presence, otherwise it uses normal arrays. Promises are definitely a no-go for this, plus it's changing stable API for no reason.

@dchest dchest closed this Aug 17, 2016
@Max13
Copy link
Author

Max13 commented Aug 17, 2016

No problem, by chance, it's my needs so I will keep it forked. What about a helper? Would you accept a helper (pscrypt) which returns a Promise and doesn't touch the current API ?

@dchest
Copy link
Owner

dchest commented Aug 17, 2016

It's just a few lines of code to convert this to Promise, so I don't think any helpers are required. (BTW, I'm working on a modern scrypt for JS as part of an open source cryptographic library, and it will indeed use promises.)

Also, promises won't allow us to create progress indicators — #2 — but not sure if this feature will ever be implemented in this package.

@Max13
Copy link
Author

Max13 commented Aug 17, 2016 via email

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