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

This module only works with node 14 or LOWER #1245

Closed
mercmobily opened this issue Feb 28, 2024 · 39 comments
Closed

This module only works with node 14 or LOWER #1245

mercmobily opened this issue Feb 28, 2024 · 39 comments
Labels

Comments

@mercmobily
Copy link

I love the work done on this module, but:

  • The module will only work with Node 14 or LOWER, because of its dependency with fs-temp 1.x
  • It's been 5 months after the last commit
  • Issues are not being answered
  • Version 2 has been "nearly done" for years

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.

@mercmobily mercmobily added the bug label Feb 28, 2024
@UlisesGascon
Copy link
Member

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: expressjs/discussions#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 :-)

@UlisesGascon UlisesGascon changed the title This module only works with node 14 or LOWER and it's possibly no longer maintained This module only works with node 14 or LOWER Feb 28, 2024
@UlisesGascon UlisesGascon pinned this issue Feb 28, 2024
@wesleytodd
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

jonchurch commented Feb 28, 2024

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
Copy link
Member

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
Copy link
Author

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
Copy link

ljharb commented Feb 29, 2024

@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
Copy link

ljharb commented Feb 29, 2024

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
Copy link
Author

mercmobily commented Feb 29, 2024

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
Copy link

ljharb commented Feb 29, 2024

"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
Copy link
Author

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
Copy link

yeya commented Mar 19, 2024

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
Copy link
Author

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
Copy link

ljharb commented Mar 19, 2024

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

@joeyguerra
Copy link

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

@joeyguerra
Copy link

@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
Copy link
Author

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
Copy link
Author

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

@mercmobily
Copy link
Author

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

@joeyguerra
Copy link

joeyguerra commented Mar 31, 2024

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
Copy link

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

@mercmobily
Copy link
Author

mercmobily commented Mar 31, 2024 via email

@joeyguerra
Copy link

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

@mercmobily
Copy link
Author

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
Copy link

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
Copy link
Author

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? nodejs/node#51977
This is behind a flag, but with a bit of luck, we will have the last obstacle out of the way....

@ljharb
Copy link

ljharb commented Apr 1, 2024

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
Copy link
Author

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

@wesleytodd
Copy link
Member

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.

@wesleytodd wesleytodd reopened this Apr 1, 2024
@joeyguerra
Copy link

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
Copy link
Member

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
Copy link
Author

mercmobily commented Apr 1, 2024

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
Copy link
Member

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
Copy link

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
Copy link
Author

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
Copy link
Author

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

@wesleytodd
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants