search

openid / AppAuth-JS

JavaScript client SDK for communicating with OAuth 2.0 and OpenID Connect providers.
Apache License 2.0
985 stars 161 forks source link

crypto-utils: Replace TextEncoder dependency #87

Closed someone1 closed 6 years ago

someone1 commented 6 years ago

Performance bump too (not that this is used much in the library): https://jsperf.com/str2ab-vs-textencoder

EDIT Actually, the TextEncoder performance in Chrome is weak, its much faster in FireFox. See this bug for details in Chrome.

Fixes #83

someone1 commented 6 years ago

I've signed the CLA, thanks for the feedback and please let me know if there's anything else!

someone1 commented 6 years ago

FYI - You have one failing test related to the test code "challengechallengechallengechallenge" - its fails as follows: "FAILED Crypto Utils Tests. produces the right base64 encoded challenge" - the code needs to be between 43 and 128 characters. This is unrelated to the changes I introduced.

Does it make sense to add some sort of CI to this repository so it can auto-run tests on every PR?

tikurahul commented 6 years ago

Thanks for letting me know about the test. I knew about it, but have been swamped with other things.

The PR looks good. :+1:

uifox commented 5 years ago

Hey @tikurahul, Can you please let me know when this change will be published to the npm repo? We were also facing the unavailable TextEncoder problem and I ended up pointing to the HEAD version of AppAuth-JS in the github repo such that we can get this change, however we would prefer pointing to an explicit version for more predictable and reliable dependency management. Thanks.