CRNS-4: Updating dev token method (for compatibility with RN)

[vishalnarkhede]

Submit a pull request

CLA

• I have signed the Stream CLA (required).
• The code changes follow best practices
• Code changes are tested (add some information if not applicable)

Description of the pull request

If devToken is used on React Native, then window.btoa is not available (for obvious reasons). Thus we need some alternative ... hence adding a common method compatible with all clients.

[ferhatelmas]

@vishalnarkhede I think copying code is bad since we won't get changes to the copied code.

Isn't it better to add buffer as a dep into RN since the previous implementation is cleaner. Unless this is possible, adding base64 as a dep here.

[vishalnarkhede]

@vishalnarkhede I think copying code is bad since we won't get changes to the copied code.

Isn't it better to add buffer as a dep into RN since the previous implementation is cleaner. Unless this is possible, adding base64 as a dep here.

@ferhatelmas My thoughts behind it!!

1. I don't think we are expecting regular changes in encoding.
2. We can definitely add dependency to our RN sdk, But people don't necessarily use our RN sdk. They can have their own react-native app (without using our sdk)
3. Buffer is a core node api, so installing it will come with loads of other dependencies, which i don't think we really need. These are quite heavy apis.

[ferhatelmas]

@ferhatelmas My thoughts behind it!!

Thanks for the detailed explanation.

I don't think we are expecting regular changes in encoding.

I wish so but the package is at v0.1.0 so not stable, I believe. Also, there are better packages like universal-base64.

We can definitely add dependency to our RN sdk, But people don't necessarily use our RN sdk. They can have their own react-native app (without using our sdk)

Then, it should be their responsibility to sign the token as they want.

Buffer is a core node api, so installing it will come with loads of other dependencies, which i don't think we really need. These are quite heavy apis.

Yes, (~7KB gzipped), that's why it's the less preferred solution. My suggestion is adding a dependency and using its api, instead of copying its source.

See the above suggested package, for example but bynens' base64 or something else doesn't matter much as soon as we can update easily on changes.

[vishalnarkhede]

@ferhatelmas universal-base64 won't work for our purpose. If you check the source code, it's doing exactly what we were doing so far - btoa for browser and Buffer for rest.

And the reason I went for copying the function is because of this performance issue issue raised on github repo - mathiasbynens/base64#19

This issue is fixed in this PR (which is not merged yet) - mathiasbynens/base64#20

So I have used a code from this PR. I think then instead of adding it directly as dependency, we can create a fork of it and publish NPM package on our end for the same. But I am just not sure if its that necessary!!

What do you think? Also @tbarbugli

[ferhatelmas]

This exchange is educating for me, thanks. We're iterating and finding the best solution.

I think it's unlikely to expect that PR to be merged in the current state because it is simply rewriting the whole source (fixes indentation, uses newer js, breaks old module syntax instead of fixing only the bug and there is an error in the implementation as you commented)

In this PR, my concern is copying. Two drawbacks of copying I see:

• harder to get updates from upstream (forks somehow better because there is no other surrounding irrelevant code at least)
• multiple copies in our projects (js and rn) and changes might diverge

Your suggestion sounds better to me since it resolves the second point of mine. When upstream is updated, we can update one line in package.json and good to go.

On the other hand, to resolve the first point too. You might check other packages that work with RN by default. The package I mentioned was just an example. Since it's a core piece, there should be something (or we can even have ours).

[tbarbugli]

@vishalnarkhede why not https://github.com/facebook/react-native/blob/1e8f3b11027fe0a7514b4fc97d0798d3c64bc895/Libraries/Utilities/binaryToBase64.js ?

[vishalnarkhede]

@tbarbugli @ferhatelmas Done!!

[ferhatelmas]

@vishalnarkhede it seems this PR does multiple things in addition to base64. Can we extract them?

[vishalnarkhede]

@ferhatelmas those other changes are very small. Don't think they are worth going through trouble of separating !!

[ferhatelmas]

@vishalnarkhede I think yes for better tracking.

For example, you fixed one of the typescript issues but it's actually a completely different fix and in its fix PR, it should close #216.