Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
refactor(minimal ops) refactor minimal ops & rename to buffer ops #9719
refactor(minimal ops) refactor minimal ops & rename to buffer ops #9719
Changes from all commits
f1f7b89
b21b1d8
5ea8654
70263d8
22574c0
0caf93d
15f71a2
30bd9c8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely sure, but I have a feeling that using arrow function here might be less performant than having a named function definition before
opAsync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opResultParser is passed into this arrow function, so that makes it hard to extract this arrow function from this opAsync function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a solution could be to call this function on an object that stores this opResultParser function, but I found this quickly makes the code a lot less readable. please let me know if you think this is useful (I could create such a solution and share it with you for review)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inteon is it required to pass that
opResultParser
? I think you could just directly call it in such functionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #9457, instead of passing
bufferOpParseResult
as argument, I passjsonOpParseResult
. This way code can be shared between jsonOps and bufferOps.https://github.com/inteon/deno/blob/2f9a9cabd0d7c48f440677ff21885eac30d22ce3/core/core.js#L320-L334
https://github.com/inteon/deno/blob/2f9a9cabd0d7c48f440677ff21885eac30d22ce3/core/core.js#L364-L378
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, let's land it as is then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also wondering if this should be called only once, otherwise a new callback is set with each call to the op
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think this should be called once. The current jsonOp implementation also does this wrongly (so it is somewhat unrelated to this PR):
deno/core/core.js
Lines 214 to 217 in bd961c3
I do fix this for both bufferOps and jsonOps in #9457.