expressjs / multer

Node.js middleware for handling `multipart/form-data`.
MIT License
11.63k stars 1.06k forks source link

This module only works with node 14 or LOWER #1245

Closed mercmobily closed 7 months ago

mercmobily commented 9 months ago

I love the work done on this module, but:

Now... I just had to add this to a production machine, to "fix" the issue with fs-temp:

"postinstall": "sed -i 's/WriteStream.call(this, null, options)/WriteStream.call(this, \"\", options)/g' node_modules/fs-temp/lib/write-stream.js"

If the module is no longer maintained, please say so and ask for somebody to take it over.

UlisesGascon commented 9 months ago

Hi @mercmobily! Thanks for open this issue, currently there is a plan to maintain this module and many others from the Express.js ecosystem, see: https://github.com/expressjs/discussions/issues/160. I will check the multer status with the TC team, so we can prioritize it. đź‘Ť

BTW feel free to join us in the #express channel in the OpenJS foundation Slack if you want to get more involved in the project :-)

wesleytodd commented 9 months ago

Additionally to what @UlisesGascon mentioned above, if you would like to help us pick things back up and get development revived we would love your help! If It looks like you might have a fix in mind, would you be able to open a PR to fix that?

mercmobily commented 9 months ago

I am normally the first guy who puts his hand up to help in any way -- especially a project as awesome as Express. But, at the moment I am in Spain (not my country) and extremely time-strapped... I am sorry. I didn't realise (or, better, I had forgotten) Multer was part of the Express ombrella. I am sorry if I was a pain when I created this ticket... I think the most important thing to do is to make sure that it uses a new version of fs-temp (2.0 fixes the Node Version problem) and update any old packages that can create security problems.

Sorry if I can't be of much more help.

wesleytodd commented 9 months ago

No worries at all @mercmobily! This is a larggely volunteer effort and we totally understand time constraints. And it is not a pain at all, we rely on community members raising issues like this to know what to focus on, keep doing so!

make sure that it uses a new version of fs-temp (2.0 fixes the Node Version problem) and update any old packages that can create security problems.

We are working toward updates which would unblock some major version upgrades in these packages and will see if this is one of those blocked by old version support. Just might not be something we can get to immediately without someone to volunteer to steward it. Maybe @LinusU can, but I think he is pretty busy right now so might not be available (again, it's all volunteer work so this is just sometimes how it goes). If your company is in need of this update, I would suggest sponsoring the folks working on this stuff.

mercmobily commented 9 months ago

If your company is in need of this update, I would suggest sponsoring the folks working on this stuff.

I didn't think of this. I will ask.

jonchurch commented 9 months ago

For reference, fs-temp is also maintained by Linus.

Here is the changelog for 2.0.0

Of note, minimum node support is 12.20.0 and the package is now ESM.

wesleytodd commented 9 months ago

Being ESM (both v2 here and in fs-temp) is likely going to be an issue isn't it? I don't see any tooling to dual publish, and as a project we do not have guidance on publishing CJS/ESM/dual but I think it is likely that we need that. Obviously I wouldn't want to block active work, but I would be quite disappointed if the project stopped supporting CJS users.

mercmobily commented 9 months ago

Any CJS project can import/use an ESM project. That's why there is no required tooling to publish both. In fact, using an ESM module from a CJS project requires tricketr, whereas using a CJS module from an EJS project is basically completely transparent. Talking about "supporting CJS developers" in 2024, given the above, isn't really that meaningful.

Same for node support: anybody using Node 10 really, and has reached EOL since April 2021.

Please note that many, many NPM package maintainers have indeed moved to ESM without even telling anybody -- and nobody basically realised, even CJS projects, since the usage was still identical.

ljharb commented 9 months ago

@mercmobily not synchronously it can't - only with import(), which is async.

importing CJS in node Just Works, period - so it's the easiest possible thing.

ljharb commented 9 months ago

It is a breaking change to go ESM-only, and for packages that dual publish, CJS users are using the CJS, not the ESM, which means they didn't "move" to ESM. The small number of maintainers that have gone ESM-only have caused tons of pain for users, and you can look at literally any of their per-major-version download numbers to see how few users actually end up adopting ESM-only versions.

mercmobily commented 9 months ago

Ah yes, I am thinking more like a consumer, rather than a provider, in terms of produced software...

You are right in that developers using CJS will find it painful to use Multer if it's only available as ESM. Multer being something used by loads of people... the sheer number of people you upset is enough to create riots!

Dual publishing is also hell-ish and non-trivial.

At the same time, EMS is the way to go, and the number of EJS-only projects will be more and more.

For something like Multer I agree, it's a very difficult situation.

(As a side note, I actually ported a huge enterprise application to EJS in less than a day, when I thought that Multer 2 was going to be released shortly!).

[Edit: I wrote Express instead of Multer... multiple times!]

ljharb commented 9 months ago

"ESM is the way to go" is a statement that I've yet to see any compelling evidence behind. You can author in ESM all you like (and use a transpiler), but why should anyone care if you ship that ESM?

Also, "will be more and more" - i disagree. It's already increasing VERY slowly for syntax that everyone has used for a decade and that everyone's excited about, and imo that's entirely because using it in packages hurts users. Do it in your apps, certainly! but there's just no benefit to doing it in a package, and tons of downsides, and that's likely to continue forever (or until the JS spec and node and browsers agree to make a change that allows requiring ESM)

mercmobily commented 9 months ago

and that's likely to continue forever (or until the JS spec and node and browsers agree to make a change that allows requiring ESM

You are right.

Not being able to "require" EMS from CJS is what makes packages tragic. I think they assumed there would be a huge wave of EJS and CJS would have been a "thing of the past" much more quickly than is actually happened (or is actually happening)

yeya commented 8 months ago

you can look at literally any of their per-major-version download numbers to see how few users actually end up adopting ESM-only versions.

Check got for example here

There is 6M weekly downloads for v11 that is "no longer maintained and we will not accept any backport requests"! Just because it is the latest version before moving to ESM.

mercmobily commented 8 months ago

you can look at literally any of their per-major-version download numbers to see how few users actually end up adopting ESM-only versions.

Check got for example here

There is 6M weekly downloads for v11 that is "no longer maintained and we will not accept any backport requests"! Just because it is the latest version before moving to ESM.

I am not sure this is a good example. After a quick awk/openoffice, you can see that version 11 has 8,025,208 weekly downloads, while all newer versions (12 up) have 3,455,238 weekly downloads.

I didn't count older versions -- if somebody is using version 10 or lower, they wouldn't be upgrading either way.

ljharb commented 8 months ago

@mercmobily https://majors.nullvoxpopuli.com/q?packages=got shows the problem more clearly.

joeyguerra commented 8 months ago

@mercmobily can you please describe the scenario? I want to write a failing test and submit a fix for it.

joeyguerra commented 8 months ago

@mercmobily I've been reviewing the code and the rc 2 version. fs-temp is a dev dependency and appears to be only used when running the tests. They all pass in Node 16 and 20+. I'm curious about the scenario the app your running is in so we can prioritize work on this repo better.

RE: the multer Release Candidate is written as an ESM: For refrence, I tested the following code with multer@2.0.0-rc.4 on Node version v16.20.2 on MacOS.

var express = require('express');
var app = express();
var http = require('http').Server(app);
var fs = require('fs');
import('multer').then(multer => {
    multer = multer.default
    var upload = multer()

    app.get('/', async (req, res) => {
        res.sendFile(__dirname + '/testing.html')
    })
    app.post('/', upload.single('small0'), async (req, res) => {
        req.file.stream.pipe(fs.createWriteStream(`uploads/${req.file.originalName}`))
        res.send('ok')
    })
    http.listen(3001)
    console.log('Server listening on port http://localhost:' + http.address().port)
})
mercmobily commented 8 months ago

It took me a while to make it happen... Here is the full stack.

If you look at the patch I am manually running, it boils down to path not accepting null anymore. fs-temp has been fixed ever since, but you guys are using an old version.

I don't think only tests use it -- this line in the stack below shows:

    at Busboy.<anonymous> (/home/merc/Development/compAs/server/node_modules/multer/lib/read-body.js:70:27)

Full stack:

UNHANDLED REJECTION! TypeError: The "path" argument must be of type string or an instance of Buffer or URL. Received null
    at TempWriteStream.WriteStream (node:internal/fs/streams:340:5)
    at new TempWriteStream (/home/merc/Development/compAs/server/node_modules/fs-temp/lib/write-stream.js:6:15)
    at Object.createWriteStream (/home/merc/Development/compAs/server/node_modules/fs-temp/lib/temp.js:121:10)
    at Busboy.<anonymous> (/home/merc/Development/compAs/server/node_modules/multer/lib/read-body.js:70:27)
    at Busboy.emit (node:events:514:28)
    at Busboy.emit (/home/merc/Development/compAs/server/node_modules/busboy/lib/main.js:37:33)
    at PartStream.<anonymous> (/home/merc/Development/compAs/server/node_modules/busboy/lib/types/multipart.js:214:13)
    at PartStream.emit (node:events:514:28)
    at HeaderParser.<anonymous> (/home/merc/Development/compAs/server/node_modules/dicer/lib/Dicer.js:50:16)
    at HeaderParser.emit (node:events:514:28)
    at HeaderParser._finish (/home/merc/Development/compAs/server/node_modules/dicer/lib/HeaderParser.js:68:8)
    at SBMH.<anonymous> (/home/merc/Development/compAs/server/node_modules/dicer/lib/HeaderParser.js:40:12)
    at SBMH.emit (node:events:514:28)
    at SBMH._sbmh_feed (/home/merc/Development/compAs/server/node_modules/streamsearch/lib/sbmh.js:159:14)
    at SBMH.push (/home/merc/Development/compAs/server/node_modules/streamsearch/lib/sbmh.js:56:14)
    at HeaderParser.push (/home/merc/Development/compAs/server/node_modules/dicer/lib/HeaderParser.js:46:19)
    at Dicer._oninfo (/home/merc/Development/compAs/server/node_modules/dicer/lib/Dicer.js:196:25)
    at SBMH.<anonymous> (/home/merc/Development/compAs/server/node_modules/dicer/lib/Dicer.js:126:10)
    at SBMH.emit (node:events:514:28)
    at SBMH._sbmh_feed (/home/merc/Development/compAs/server/node_modules/streamsearch/lib/sbmh.js:188:10)
    at SBMH.push (/home/merc/Development/compAs/server/node_modules/streamsearch/lib/sbmh.js:56:14)
    at Dicer._write (/home/merc/Development/compAs/server/node_modules/dicer/lib/Dicer.js:108:17)
    at writeOrBuffer (node:internal/streams/writable:556:12)
    at _write (node:internal/streams/writable:490:10)
    at Writable.write (node:internal/streams/writable:494:10)
    at Multipart.write (/home/merc/Development/compAs/server/node_modules/busboy/lib/types/multipart.js:291:24)
    at Busboy._write (/home/merc/Development/compAs/server/node_modules/busboy/lib/main.js:80:16)
    at writeOrBuffer (node:internal/streams/writable:556:12)
    at _write (node:internal/streams/writable:490:10)
    at Writable.write (node:internal/streams/writable:494:10)
    at IncomingMessage.ondata (node:internal/streams/readable:985:22)
    at IncomingMessage.emit (node:events:514:28)
    at Readable.read (node:internal/streams/readable:758:10)
    at flow (node:internal/streams/readable:1248:53)
    at resume_ (node:internal/streams/readable:1227:3)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) PROMISE: Promise {
  <rejected> TypeError: The "path" argument must be of type string or an instance of Buffer or URL. Received null
      at TempWriteStream.WriteStream (node:internal/fs/streams:340:5)
      at new TempWriteStream (/home/merc/Development/compAs/server/node_modules/fs-temp/lib/write-stream.js:6:15)
      at Object.createWriteStream (/home/merc/Development/compAs/server/node_modules/fs-temp/lib/temp.js:121:10)
      at Busboy.<anonymous> (/home/merc/Development/compAs/server/node_modules/multer/lib/read-body.js:70:27)
      at Busboy.emit (node:events:514:28)
      at Busboy.emit (/home/merc/Development/compAs/server/node_modules/busboy/lib/main.js:37:33)
      at PartStream.<anonymous> (/home/merc/Development/compAs/server/node_modules/busboy/lib/types/multipart.js:214:13)
      at PartStream.emit (node:events:514:28)
      at HeaderParser.<anonymous> (/home/merc/Development/compAs/server/node_modules/dicer/lib/Dicer.js:50:16)
      at HeaderParser.emit (node:events:514:28)
      at HeaderParser._finish (/home/merc/Development/compAs/server/node_modules/dicer/lib/HeaderParser.js:68:8)
      at SBMH.<anonymous> (/home/merc/Development/compAs/server/node_modules/dicer/lib/HeaderParser.js:40:12)
      at SBMH.emit (node:events:514:28)
      at SBMH._sbmh_feed (/home/merc/Development/compAs/server/node_modules/streamsearch/lib/sbmh.js:159:14)
      at SBMH.push (/home/merc/Development/compAs/server/node_modules/streamsearch/lib/sbmh.js:56:14)
      at HeaderParser.push (/home/merc/Development/compAs/server/node_modules/dicer/lib/HeaderParser.js:46:19)
      at Dicer._oninfo (/home/merc/Development/compAs/server/node_modules/dicer/lib/Dicer.js:196:25)
      at SBMH.<anonymous> (/home/merc/Development/compAs/server/node_modules/dicer/lib/Dicer.js:126:10)
      at SBMH.emit (node:events:514:28)
      at SBMH._sbmh_feed (/home/merc/Development/compAs/server/node_modules/streamsearch/lib/sbmh.js:188:10)
      at SBMH.push (/home/merc/Development/compAs/server/node_modules/streamsearch/lib/sbmh.js:56:14)
      at Dicer._write (/home/merc/Development/compAs/server/node_modules/dicer/lib/Dicer.js:108:17)
      at writeOrBuffer (node:internal/streams/writable:556:12)
      at _write (node:internal/streams/writable:490:10)
      at Writable.write (node:internal/streams/writable:494:10)
      at Multipart.write (/home/merc/Development/compAs/server/node_modules/busboy/lib/types/multipart.js:291:24)
      at Busboy._write (/home/merc/Development/compAs/server/node_modules/busboy/lib/main.js:80:16)
      at writeOrBuffer (node:internal/streams/writable:556:12)
      at _write (node:internal/streams/writable:490:10)
      at Writable.write (node:internal/streams/writable:494:10)
      at IncomingMessage.ondata (node:internal/streams/readable:985:22)
      at IncomingMessage.emit (node:events:514:28)
      at Readable.read (node:internal/streams/readable:758:10)
      at flow (node:internal/streams/readable:1248:53)
      at resume_ (node:internal/streams/readable:1227:3)
      at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
    code: 'ERR_INVALID_ARG_TYPE'
  }
}
mercmobily commented 8 months ago

Wait wait I am using 2.0.0-rc.1 -- could the dependency be fixed in rc2?

mercmobily commented 8 months ago

(Wait... I didn't know there was an rc4?!?)

joeyguerra commented 8 months ago

Maybe. multer@2.0.0-rc.4 depends on fs-temp@2.0.1. Are you able to update to multer@2.0.0-rc.4 and test again?

joeyguerra commented 8 months ago

btw, @mercmobily how is your app importing multer@2+ since it's an ESM?

mercmobily commented 8 months ago

My app is also ESM -- I updated everything basically just for multer about a year ago.

How do I get rc4?

On Sun, 31 Mar 2024, 5:20 am Joey Guerra, @.***> wrote:

btw, @mercmobily https://github.com/mercmobily how is your app importing @.***+ since it's an ESM?

— Reply to this email directly, view it on GitHub https://github.com/expressjs/multer/issues/1245#issuecomment-2028540105, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAQHWXXMNMYG24SLIWSMN4LY256HXAVCNFSM6AAAAABD5XASXGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRYGU2DAMJQGU . You are receiving this because you were mentioned.Message ID: @.***>

joeyguerra commented 8 months ago

@mercmobily npm i multer@2.0.0-rc.4 should install it.

mercmobily commented 8 months ago

Oh sweet, I don't need to do last-minute patching anymore...! What is the plan for V2? Are you guys going to release this RC? Or are you going to convert everything back to CJS before release?

joeyguerra commented 7 months ago

Two big questions. I just now started trying to help. So I have some learning to do and don’t know what the plan is yet for multer. But I have confidence there will be one.

I wish we could get everyone who’s actively using multer on an email list and start communicating the changes, re: ESM, in hopes to get feedback to drive a direction. I wonder how many people would respond to a survey? We could host a survey page on the express site 🤓

Perhaps a safer approach would be to update v1 dependencies to their latest versions and go ahead and release v2 as it stands. That would hopefully give users flexibility to evolve their codebases on their time table.

Glad you found a better solution for your situation. It’s funny how it turns out that keeping software soft can be quite difficult.

mercmobily commented 7 months ago

There is a certain anti-ESM sentiment. I had some discussions with the Express guys. They are worried that adoption and upgrading will slow down if it's released as an ESM. However... are you aware of this that was recently merged into Node? https://github.com/nodejs/node/pull/51977 This is behind a flag, but with a bit of luck, we will have the last obstacle out of the way....

ljharb commented 7 months ago

At the moment, an ESM-only package empirically and drastically limits adoption.

Once that feature is released, unflagged, in every supported node version, and a few years have passed, that may change - but package authors should not be holding their breath for that.

mercmobily commented 7 months ago

I guess we will talk about it in 2034...

wesleytodd commented 7 months ago

Hey @mercmobily, I understand maybe you are frustrated here but this kind of thing is not helpful. We are trying to get more contributors on boarded to the project to help prevent the situation these packages are in from continuing or ever happening again. If you would like to work with us on that we would gladly have your help. But closing the issue with that type of comment is not productive.

@joeyguerra If I understand the situation correctly @LinusU was interested in still working on this but has been pretty busy. Maybe we could get just some of his time soon to help you and onboard so we can start getting some of this stuff fixed up? I personally need to focus on the v5 express release, but I believe that having this package updated for current node versions and then also ready to support v5 is quite important. If you are interested in helping make this side of that work happen it would be AWESOME and very helpful.

joeyguerra commented 7 months ago

Yes. I’m willing and able to help. @LinusU I can follow your lead here. My schedule is flexible.

Perhaps we can create a separate issue to use for hashing out the plan?

wesleytodd commented 7 months ago

Yeah whatever works best for @LinusU I think. I just wanted to chime in to make sure this issue didn't stall out if I could help anything.

mercmobily commented 7 months ago

I am sorry, there was a communication issue here. I closed this issue because this issue is about the module not working on Node 14 or lower -- which is... not true! I should have called it "Inept development (me) is trying to use RC1 instead of RC4"... which didn't sound as flattering for me!

Plus:

I guess we will talk about it in 2034...

This was NOT a disgruntled complaint! It was really my forecast. What I should have written is:


that feature is released, unflagged, in every supported node version, and a few years have passed,

I guess that's going to be around 2034 at least. So, at least for now, I assume you should revert the module to CJS and wait a few years...


In either case, the issue at hand (it doesn't work with modern Node) really is resolved, which is why I closed it. [Plus, I think it would be better to have future important discussions on a more suitable issue, especially since the issue I created is basically fixed with "Just install RC4"... :D

Sorry for the noise and for the misunderstanding. But please rest assured that I am not "that" kind of user/developer...!

wesleytodd commented 7 months ago

Ah ok, sorry for the misunderstanding! We can close this then, sorry for re-opening. Seriously though it would be great for you to help us figure out what next steps need to happen for this repo. Even if that is as a user working closely with us to ensure that the 2.x beta's are working so we can fix forward!

joeyguerra commented 7 months ago

I’m glad you created this issue. Among many things, it shows that at least 1 person is using multer and express for something valuable and I think that’s amazing. I’m glad we came up with a better solution to the situation together. I don’t know about y’all, but I suffer from major imposter syndrome when trying to use code I didn’t write and these types of online interactions help me make progress.

Side note: the internet and programming is supposed to be fun, first and foremost. Here’s hoping to bring back the fun.🤩

mercmobily commented 7 months ago

Among many things, it shows that at least 1 person is using multer and express for something valuable and I think that’s amazing.

Well... what can I say, I feel I am representing one of the 4 mil people who downloaded multer this week!!!

image

mercmobily commented 7 months ago

Seriously though, we are using on a huge production project, and it's worked really well for years. Coming up to 7 now.

wesleytodd commented 7 months ago

Yeah, the impact of these projects the Express org owns is quite large. I guess I should have mentioned above on the topic of user outreach that we really don't need to do that to get a feel for usage. As an example to add on to @mercmobily's usage, I just checked and we have about 20 apps installing multer with commits in the last year at Netflix. This package is absolutely a critical dependency to keep working and healthy.

KostyaTretyak commented 3 months ago

Hi guys! To all those who are looking for multer for new versions of Node.js, I can recommend the package @ts-stack/multer (I am its author). This fork is based on multer v2.0.0-rc.4, it updates almost all dependencies to new versions. By the way, fs-temp has been replaced by tempy. Note that this package returns a Promise instead of middleware, and it has a bug fixed.

Required Node.js >= v20.0.6, writen in TypeScript.