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.

Sign up for GitHub

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

Jump to bottom

Add Base64Url encoding #56

Merged
merged 3 commits into from
Jul 5, 2023
Merged

Add Base64Url encoding #56

merged 3 commits into from
Jul 5, 2023

Conversation

JordanMartinez
Copy link
Contributor

@JordanMartinez JordanMartinez commented Jul 5, 2023 •
edited
Loading

Description of the change

  • Add Base64Url encoding.

Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@JordanMartinez JordanMartinez changed the title update encoding Add Base64Url encoding; drop aliased encodings Jul 5, 2023
@JordanMartinez
Copy link
Contributor Author

@thomashoneyman 🏓

@thomashoneyman
Copy link
Contributor

Has Node indicated that they will drop these aliases? What harm do you see in retaining them?

@JordanMartinez
Copy link
Contributor Author

Has Node indicated that they will drop these aliases? What harm do you see in retaining them?

The UCS2 one not so much, but the binary one is confusing. Per Node docs

'binary': Alias for 'latin1'. See binary strings for more background on this topic. The name of this encoding can be very misleading, as all of the encodings listed here convert between strings and binary data (emphasis mine). For converting between strings and Buffers, typically 'utf8' is the right choice.

@JordanMartinez
Copy link
Contributor Author

Regardless, this is one of those "controversial" changes, and I don't want to be blocked by this. I'll revert the commit.

@JordanMartinez JordanMartinez changed the title Add Base64Url encoding; drop aliased encodings Add Base64Url encoding Jul 5, 2023
@JordanMartinez JordanMartinez merged commit c447cb5 into master Jul 5, 2023
1 check passed
@JordanMartinez JordanMartinez deleted the update-encoding branch July 5, 2023 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomashoneyman thomashoneyman thomashoneyman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JordanMartinez @thomashoneyman