Import Node.js APIs with node: prefix#349
Import Node.js APIs with node: prefix#349lacolaco wants to merge 4 commits intomodelcontextprotocol:mainfrom lacolaco:import-node-buffer
node: prefix#349Conversation
lacolaco
changed the title
Buffer from node:bufferexplicitlyBuffer from node:buffer explicitly
|
I tested this with Deno and it worked. |
ihrpr
left a comment
ihrpr
left a comment
There was a problem hiding this comment.
Thank you for your contribution and sorry for the delays in PR reviews. Importing type seems off in stdio, please can you check?
| @@ -1,3 +1,4 @@ | |||
| import type { Buffer } from "node:buffer"; | |||
There was a problem hiding this comment.
probably because it's only used as type in https://github.com/lacolaco/mcp-typescript-sdk/blob/import-node-buffer/src/server/stdio.ts#L27
There was a problem hiding this comment.
Exactly. In this file Buffer is referred only as an argument type.
There was a problem hiding this comment.
it's also used directly. this._readBuffer.append(chunk)
How its been tested?
There was a problem hiding this comment.
That ReadBuffer you mention is imported from shared/stdio.js, which is modified to use node:buffer.
What this Issue is concerned with and resolves is a direct reference to the global variable Buffer. It is fine if the object of type Buffer is derived from node:buffer.
There was a problem hiding this comment.
is there a way to add a test to prevent regressions?
There was a problem hiding this comment.
@ihrpr Is the test intended to prevent regression in a Node.js environment? Or is it intended to test for viability in deno?
There was a problem hiding this comment.
a test to make sure if anyone adds any new use of Buffer (or any other way to break compatibility) sdk will still be compatible with Deno
lacolaco
changed the title
Buffer from node:buffer explicitlynode: prefix
|
@ihrpr Could you re-review for this PR? |
|
hi @domdomegg, could this PR be merged since it's been approved? the requested changes seems to already been addressed in 0e1c586 |
.github/workflows/main.yml
Outdated
| - run: npm test | ||
| - run: npm run lint | ||
|
|
||
| check-deno-compatibility: |
There was a problem hiding this comment.
Sorry about the delays in review, I think this check is good by way too specific for deno. Let's remove it and then we can merge
felixweinberger
force-pushed
the
main
branch
from
0845a57 to
c94ba4b
Compare
bhosmer-ant
added
the
needs maintainer action
|
Closing this since. #1187 merged |
|
I'm glad the issue was eventually resolved, but I'm disappointed that it was left unresolved for several months after I reported the problem and even responded to review requests on the pull request. Furthermore, another pull request was merged within a few hours, effectively closing my issue, but there was no acknowledgment or apology whatsoever. I will not be contributing to this project in the future. Thank you. |
7 participants
Import
Bufferfromnode:bufferexplicitly to get deno-compatibility.Motivation and Context
How Has This Been Tested?
npm run buildandnpm testBreaking Changes
No
Types of changes
Checklist
Additional context