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

perf: use Reader.create #800

Merged
merged 2 commits into from
Mar 18, 2023
Merged

perf: use Reader.create #800

merged 2 commits into from
Mar 18, 2023

Conversation

davidzeng0
Copy link
Contributor

Allows protobufjs/minimal to detect the config and optionally use BufferReader if supported.

@davidzeng0
perf: use Reader.create
7f084ba 
Allows protobufjs/minimal to detect the config and optionally
use BufferReader if supported.
@stephenh
Copy link
Owner

@davidzeng0 this looks great, but there is a failure in the build:

Error: illegal buffer
    at create_typed_array (/home/runner/work/ts-proto/ts-proto/node_modules/protobufjs/src/reader.js:47:15)
    at create_buffer (/home/runner/work/ts-proto/ts-proto/node_modules/protobufjs/src/reader.js:63:23)
    at Function.create_buffer_setup (/home/runner/work/ts-proto/ts-proto/node_modules/protobufjs/src/reader.js:64:15)
    at /home/runner/work/ts-proto/ts-proto/integration/simple/simple.ts:2072:78
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

Disclaimer I've not looked in to it this at all, but can you tell if it's something wrong we're doing in the test, or maybe some wrinkle about Reader.create that we'll have to handle? Thanks!

@davidzeng0
Copy link
Contributor Author

@stephenh It seems like the test is returning the mock value {} and it failing because an object isn't a buffer. I'm not entirely sure how tests before this commit passed, but I'll look into it

@davidzeng0
Copy link
Contributor Author

davidzeng0 commented Mar 18, 2023 •
edited
Loading

Yeah, so new Reader({}) succeeds because there are no type checkings, however Reader.create fails

image

Root cause on line 6
image

The easy fix is to replace {} with Buffer.from([])

@stephenh
Copy link
Owner

@davidzeng0 Ah good find! Do you mind pushing a fix up to this PR?

@davidzeng0
Copy link
Contributor Author

@stephenh Fixed

Also, any progress on the other 2 PRs?

Thanks.

@stephenh stephenh merged commit 869e448 into stephenh:main Mar 18, 2023
@davidzeng0 davidzeng0 deleted the create branch March 18, 2023 17:52
stephenh pushed a commit that referenced this pull request Mar 18, 2023
@semantic-release-bot
chore(release): 1.142.1 [skip ci]
e36c8c6 
## [1.142.1](v1.142.0...v1.142.1) (2023-03-18)

### Performance Improvements

* use Reader.create ([#800](#800)) ([869e448](869e448))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.142.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@davidzeng0 @stephenh