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

Check for NativeBuffer in ed25519#messageToNativeBuffer #698

Merged
merged 1 commit into from
Sep 5, 2019

Conversation

TimWolla
Copy link
Contributor

While this technically is not necessary, because node.js' Buffer
inherits from Uint8Array this contract is broken by jest.
See jestjs/jest#4422.

Add a message instanceof NativeBuffer to the function to fix the issue,
it does no harm and technically would be the more correct check, because
for node.js passing a raw Uint8Array does not return a Buffer.


I can also adjust the function to wrap the Uint8Array in a Buffer, if NativeBuffer === Buffer to be super correct. Just let me know.

While this technically is not necessary, because node.js' Buffer
inherits from Uint8Array this contract is broken by jest.
See jestjs/jest#4422.

Add a `message instanceof NativeBuffer` to the function to fix the issue,
it does no harm and *technically* would be the more correct check, because
for node.js passing a raw Uint8Array does not return a Buffer.
@dlongley dlongley requested a review from davidlehn July 23, 2019 14:53
@TimWolla
Copy link
Contributor Author

TimWolla commented Sep 2, 2019

Any update about this? 😃

@davidlehn
Copy link
Member

I'll pull this in since we're doing a release of related code. Seems like maybe that check should just be for NativeBuffer. Would be nice to have some tests for this if anyone wants to figure that out.

@davidlehn davidlehn merged commit 1990634 into digitalbazaar:master Sep 5, 2019
@TimWolla TimWolla deleted the message-to-native-buffer branch September 5, 2019 08:04
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.

3 participants