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

Broken compatibility with Node.js 16.0 #5

Closed
pubmikeb opened this issue Apr 7, 2021 · 8 comments · Fixed by NeuraLegion/multer#23
Closed

Broken compatibility with Node.js 16.0 #5

pubmikeb opened this issue Apr 7, 2021 · 8 comments · Fixed by NeuraLegion/multer#23

Comments

@pubmikeb
Copy link

pubmikeb commented Apr 7, 2021

fs‑temp passes null as the path parameter:

WriteStream.call(this, null, options)
which breaks the compatibility with Node.js 16.0 and leads to the crash when using fs‑temp:

node:internal/process/promises:245
          triggerUncaughtException(err, true /* fromPromise */);
          ^

TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string or an instance of Buffer or URL. Received null
    at TempWriteStream.WriteStream (node:internal/fs/streams:360:5)
    at new TempWriteStream (C:\Users\User\IdeaProjects\TMS\node_modules\fs-temp\lib\write-stream.js:6:15)
    at Object.createWriteStream (C:\Users\User\IdeaProjects\TMS\node_modules\fs-temp\lib\temp.js:121:10)
    at Busboy.<anonymous> (C:\Users\User\IdeaProjects\TMS\node_modules\multer\lib\read-body.js:70:27)
    at Busboy.emit (node:events:369:20)
    at Busboy.emit (C:\Users\User\IdeaProjects\TMS\node_modules\busboy\lib\main.js:37:33)
    at PartStream.<anonymous> (C:\Users\User\IdeaProjects\TMS\node_modules\busboy\lib\types\multipart.js:214:13)
    at PartStream.emit (node:events:369:20)
    at HeaderParser.<anonymous> (C:\Users\User\IdeaProjects\TMS\node_modules\dicer\lib\Dicer.js:50:16)
    at HeaderParser.emit (node:events:369:20) {
  code: 'ERR_INVALID_ARG_TYPE'
}

Process finished with exit code 1

At Node.js thread I've got a response that the change nodejs/node#35187 has broken some compatibility and fs-temp has to be adjusted to be able to work with Node.js 16.0+

Update:
Node.js 16.0 has been officially released and neither fs‑temp, nor any depending on it module don't support Node.js 16.0.

@pubmikeb pubmikeb changed the title fs-temp doesn't work with Node.js 16.0 due to semver-major upgrade Broken compatibility with Node.js 16.0 Apr 24, 2021
@pubmikeb
Copy link
Author

pubmikeb commented Apr 26, 2021

@LinusU, after a brief codebase review, I assume that it is enough to pass "null" to WriteStream.call(…) instead of null to fix a broken compatibility with the Node.js 16.0. I've tried to propose a pull request, but I don't have permissions.

Could you, please, review this issue to ensure the support of Node.js 16.0 by fs-temp?
Thanks!

@pubmikeb
Copy link
Author

pubmikeb commented May 8, 2021

@LinusU, I've prepared a fix how can I propose it for the project?
It looks like I don't have a permission to propose a PR.

Without this fix I can't upgrade the system to Node.js 16.x.

Thanks.

@ExE-Boss
Copy link

ExE-Boss commented May 8, 2021

You should just be able to open a PR from your own fork, which you need to create: https://github.com/LinusU/fs-temp/fork

@LinusU
Copy link
Owner

LinusU commented May 10, 2021

@pubmikeb what do you think about an empty string ('') instead of a string with the word "null"?

@pubmikeb
Copy link
Author

@LinusU, if an empty string ('') suits the updated signature of Node.js 16+, then it's OK.
I'm not deeply enough familiar with the codebase of fs-temp, so perhaps there is some side-effect of the change.

Let's try it and if it's OK, then the change can be released.

@pubmikeb
Copy link
Author

@pubmikeb what do you think about an empty string ('') instead of a string with the word "null"?

Right, an empty string ('') solves the issue, I've proposed a pull request.

@pubmikeb
Copy link
Author

pubmikeb commented May 13, 2021

@LinusU, the pull request with the fix is proposed, please review it and in case everything is OK, the PR can be merged.
Hopefully, https://github.com/expressjs/multer will update the dependencies shortly after that.

@LinusU
Copy link
Owner

LinusU commented Jul 24, 2021

Really sorry for the huge delay in this!!

I fixed this in f02bfe3 which is part of the 🚢 2.0.0 / 2021-07-24 release!

@LinusU LinusU closed this as completed Jul 24, 2021
denis-maiorov-brightsec added a commit to NeuraLegion/multer that referenced this issue Aug 1, 2023
Migrates to `fs-temp@2` to resolve LinusU/fs-temp#5
derevnjuk added a commit to NeuraLegion/multer that referenced this issue Aug 4, 2023
Migrates to `fs-temp@2` to resolve LinusU/fs-temp#5

Co-authored-by: Artem Derevnjuk <[email protected]>
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 a pull request may close this issue.

3 participants