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

Improve documentation of base64 function #4338

Closed
wants to merge 2 commits into from
Closed

Conversation

turt2live
Copy link
Member

No description provided.

@turt2live turt2live added A-Documentation T-Task Tasks for the team like planning labels Aug 6, 2024
@turt2live turt2live requested a review from a team as a code owner August 6, 2024 21:14
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion that ArrayBuffer is a node concept is actually a lie - it exists on web, and it's what SubtleCrypto.decrypt returns.

What is a node concept is Buffer. AFAICT, your code is currently going via the "node" path, using Webpack's polyfill of Buffer...

So yeah, the comments are crap, but not for this reason...

(Should we be relying on the polyfill of Buffer? If we can, why do we need the verbose implementation? so many questions...)

@richvdh
Copy link
Member

richvdh commented Aug 6, 2024

lol, of course the Buffer polyfill actually uses base64-js to implement its base64 (https://github.com/feross/buffer/blob/master/index.js#L11). So, despite the comment saying "let's try to avoid using base64-js", that's exactly what we end up doing...

@turt2live
Copy link
Member Author

cries

I'm not sure this is the PR to fix the situation, but if we can save future folks from an expedition of sadness, then we should. I'll open some issues about the potential optimizations when I get back to this PR.

(also thank you for the prompt review, and further research)

@richvdh
Copy link
Member

richvdh commented Aug 7, 2024

I'm not sure this is the PR to fix the situation,

Yeah, I'm not insisting you fix this right now. But doubling down on "ArrayBuffers are a node concept" line doesn't seem very helpful either, tbh. Maybe just abort this PR for now?

@dbkr
Copy link
Member

dbkr commented Aug 7, 2024

Sorry, this will have been my comment, and I suspect having got the terms clear in my head, my fingers then wrote something else. Element web and it's joyous webpack world may well be ending up in the node code, although I'm not sure that's necessarily js-sdk's job to worry about. What was the original confusion?

@turt2live
Copy link
Member Author

It wasn't clear which path was the "browser" path, and the ArrayBuffer comment just doesn't make sense - it's saying to check for something because we can't use reduce, but the line of code doesn't check anything and still uses reduce.

@turt2live
Copy link
Member Author

Closing in favour of #4341

@turt2live turt2live closed this Aug 7, 2024
@turt2live turt2live deleted the travis/b64-docs branch August 7, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Documentation T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants