expressjs / express

Fast, unopinionated, minimalist web framework for node.
https://expressjs.com
MIT License
65.57k stars 16.15k forks source link

Add bodyParser back to express 4.x #2211

Closed dougwilson closed 7 years ago

dougwilson commented 10 years ago

We have removed all the middlewares from being bundled with express (which is awesome, i.m.o), but left one: static. I think we really should bundle bodyParser again, because I think parsing bodies is probably even more common than serving static files and many people really have no idea how to read the body from IncomingMessage objects (aka req) in Node.js core for whatever reason.

jonathanong commented 10 years ago

sure why not

dougwilson commented 10 years ago

Yea. I just have to work around what do do about multipart first.

jonathanong commented 10 years ago

naw i wouldn't bother with multipart. there's no good API that pleases everyone.

dougwilson commented 10 years ago

I know, but I mean like work out what we need to tell people, etc. because otherwise having stuff and not multipart will get lots of questions and we need to be ready for that before putting in body parser :)

rlidwka commented 10 years ago

How app.use(express.bodyparser) is any different from app.use(require('body-parser'))?

Just trying to find the real issue here.

dougwilson commented 10 years ago

How app.use(express.bodyparser) is any different from app.use(require('body-parser'))?

Conversely, how is app.use(express.static) any different from app.use(require('serve-static'))? Just like how we include out of the box the ability to send files, we should provide a way to get at the body (which at a minimum would be bodyParser.raw). To a seasoned developer, it may seem trivital since you know to require('body-parser'). I see this happen over and over:

app.post('/signup', function (req, res) {
  // now what do?
  // how get body????
})

It actually seems pretty laughable that express gives you a lot of stuff but no way to get at the body. req.on('data', fn) string concatenation is not a solution for people who are not advanced; doing this is completely non-obvious.

defunctzombie commented 10 years ago

How is babby formed?

aredridel commented 10 years ago

How much of this could be solved with better documentation?

dougwilson commented 10 years ago

How much of this could be solved with better documentation?

For one, we'd basically be re-documenting Node.js core; explain streams, etc. People want to just get their form data from express and parsing the body should just be built-in. You can't reasonably think people will want to do req.on('data', fn), buffer up a data string to JSON.parse, also correctly dealing with client errors and other stuff that goes wrong during the read, etc.

I think the minimum bar express should have is something like bodyParser.raw (i.e. something that will give them a Buffer of the entire body they can do what they want with).

rlidwka commented 10 years ago

In case of express I suppose you could just teach people how to find middlewares in npm registry.

Would be something like Java, "I don't know how to multiplicate 2 by 2, but I heard there is a module for it". :P

dougwilson commented 10 years ago

Yea, but this is getting the body, a necessity for most HTTP methods. It's on the same level as saying "oh, you wanted to know the request's URL? You'll need a lib for that" ;) Without getting the body, you would not have a very interesting site (read-only).

aredridel commented 10 years ago

Yeah. But definitely has to be able to be opted out of -- a great many bodies won't fit in RAM!

aredridel commented 10 years ago

But having the feature (and a place to hook more complex parses) would be smart.

dougwilson commented 10 years ago

But definitely has to be able to be opted out of -- a great many bodies won't fit in RAM!

This is something that is opt-in not opt-out. Just like you have to opt-in to using express.static.

aredridel commented 10 years ago

Makes sense.

gabeio commented 10 years ago

imho: it's added space that can be left out. I love express, especially it's latest step with making itself more modular. If anything this should be waited on to see if people like it or dislike it being not part of express.bodyParser(). If anything you guys should wait until version 5 to put it back. It's nice to be able to write a very small static file server/template file server without the added weight of get/post variables. I use it for both of these purposes in multiple places. I even think you guys should remove the static server as I can easily make an argument that in other projects I never use the static server because I have something like nginx handle the static load faster.

dougwilson commented 10 years ago

@gabeio yes, we are waiting for now, listening to the community. Right now, this ticket is not listed in either of the two Release PRs, so we haven't even determined where it'll go :)

secretfader commented 10 years ago

For what it's worth, I like having body-parser as it's own module.

raymondfeng commented 10 years ago

IMO, it's fine for body-parser to be its own npm module physically.

From express users' perspective, what do you guys think about the approach we took at https://github.com/strongloop/loopback/blob/master/lib/express-middleware.js?

It was inspired by express (https://github.com/strongloop/express/blob/master/lib/express.js#L66). The difference is that LoopBack leans toward keeping back-compatibility while express wants folks to upgrade.

dougwilson commented 10 years ago

while express wants folks to upgrade

Express 3.x was completely maintained, supported and upgraded it's stuff with compatibility shims when necessary. We did not require anyone to stop using express 3.x, unless you're planning to change that and drop support for 3.x?

raymondfeng commented 10 years ago

@dougwilson Sorry for my poor wording :-)

Clarification: I should say express wants folks to use middlewares in the recommended way by issuing deprecation messages/throwing errors.

alonronin commented 10 years ago

-1 no Middleware should be bundled. even static files should be considered to separate from core.

Fishrock123 commented 10 years ago

@alonronin That's the plan for the next major version, kinda sorta.

Right now I don't see why we shouldn't do this for body-parser.

riston commented 10 years ago

There is no real reason why the body-parser should be included again, if the Express followed the modular application path then this issue should be closed.

@alonronin +1 for static module

kokujin commented 10 years ago

There was a lot of headaches with the bodyParser, then the search of which one to use and its configuration, it even broke Sails.js a bit.

If Its out of express core, then it should stay out.

dougwilson commented 10 years ago

There was a lot of headaches with the bodyParser

What were they? Can you elaborate?

it even broke Sails.js a bit

How so? I talk with Mike all the time and they are actually moving to use the body-parser module directly since it's more stable, not the other way around.

kokujin commented 10 years ago

Well there was the security warning, and a few mentioned that it would be better to use one of the other parsers when implementing file uploads. I also had the impression that skipper was made just for this purpose (https://github.com/balderdashy/skipper). All this was confusing at the time that we were evaluating Express and Sails for a project

Fishrock123 commented 10 years ago

@kokujin That was the old bodyParser. The new one would use body-parser.

jimmywarting commented 10 years ago

I think a parser is as much important as the static middleware. Without it we couldn't use POST, PUT and PATCH method. so there wouldn't be any reason to keep stuff like app.post() So that would then also have to be made modular?! Uploading data is made everywhere! When we removed it I think it has raised more headache and question such as:

there's no good API that pleases everyone

others have really porr documentation, coding standard and low maintenance and many issues. Some are not so well integrated with express at all and i think we could do better at building our own that hopefully pleases everyone

I guess someone have used GET method instead of post in there API when it comes to saving new entries since they don't have a parser any longer. its just wrong. GET should be used the fetch data not saving. Anyone feeling guilty here?

It actually seems pretty laughable that express gives you a lot of stuff but no way to get at the body.

I totally agree

modules is grate but i still think we should build our own module. It feels better having a grate team that knows what they are doing

defunctzombie commented 10 years ago

Disagree. I think the docs can have very clear examples of what module we recommend and how to use it. The benefits or separate module are simple and clear: updates without break the world changes.

Fishrock123 commented 10 years ago

we should just remove static (eventually express 5 tm)

bdeanindy commented 9 years ago

Hey Doug,

Not sure if you remember me or not, but IIRC...you are the most talentedJS-DecDev I've had the privilegeof knowing. That being said, I am curious about a few things regarding what you're proposing with this change.

  1. While you are quite tetalented, several developers are not quite at your level and they just follow what is prescribed. Whatimpact would a change like this bear for the devs at that level? (not trying to diminish your work...just making sure that the people who use these tools are not lost in the mix).

In closing, it is good to see you contributing to SL, and I hope you're doing well. -peace- Benjamin Dean

Fishrock123 commented 9 years ago

All it means is putting body-parser in your package.json and doing var bodyParser = require('body-parser').

dougwilson commented 9 years ago

Hi @bdeanindy ! So I had actually opened this issue because of feedback from K. Andrews :)

What impact would a change like this bear for the devs at that level?

So, the changes that I'm proposing here (and still kind of think should happen, thus why I haven't closed the issue) is to include body parsing within Express core, just like static file serving is included in core. There are really two wrinkles currently that are preventing this from happening:

  1. We definitely need some solution for multipart bodies, because it's (unfortunately) a basic type of the web. We should, if anything, support stuff you can get from a vanilla <form>.
  2. We still need (and are working towards) a way for people who want a more "light weight" Express to be able to get that. To me, the npm module "express" I consider to be "the entry point to the Express ecosystem", rather than it being the only way to actually build Express apps.

In conclusion, my goals are to keep "express" flexible, but also welcoming to new users and not force them to hunt on npm for something just to put a <form> on the site.

gabeio commented 9 years ago

Just a question (feeling noobie with this) when you require('express') is the body-parser going to be loaded into memory and only used if you use require('express').bodyParser?

dougwilson commented 9 years ago

@gabeio yes, and even then, the body-parser module only loads into memory the parser types you used, and then the urlencoded parser will only load the query string parser implementation you actually use :)

gabeio commented 9 years ago

Is there any way to modularize it more so that it DOESN'T load until you ask for it? Without making it a separate package?

dougwilson commented 9 years ago

@gabeio I'm not sure what you're asking? I just said that's what it would do: not load until you ask for it without it being a separate package.

gabeio commented 9 years ago

oh derp, sorry I read it wrong.

gabeio commented 9 years ago

I guess if it's not loading into memory then it's not really that critical that it's in the express package... even if you are using a different parser.

defunctzombie commented 9 years ago

The nice thing about not shipping with express is that we make it immediately clear to the user that middleware is how you extend express and familiarizes them with adding the middleware. Express is pretty low level so that all of the pieces can remain small(ish) and so we don't have to upgrade express when we publish new versions of the middleware. It also allows the dev to upgrade body-parser separately from express itself.

dougwilson commented 9 years ago

@defunctzombie this is also true. A good example is the recent security issues and fixes within the serve-static module--the only middleware currently distributed as part of Express 4.x. Unless people jumped to just using serve-static directory, they may have had to perform major upgrades of Express to get the security fixes.

kevinSuttle commented 9 years ago

Is this still relevant? https://gist.github.com/cerebrl/6487587#dont-use-bodyparser

dougwilson commented 9 years ago

@kevinSuttle not really, because body-parser does not write anything to your disk, and never will in the future. There are still things to consider when using a body parser, like how much data you're willing to be in the process's memory at a time, etc., and also just reducing your attack surface like not parsing bodies on routes that require authentication until after the authentication has been confirmed, etc.

Remember, express.bodyParser is a completely different module than the newer body-parser npm module, which is what would be re-integrated with Express.

dougwilson commented 9 years ago

The actual contents on the warning are pretty sound, of course, though dated to Express 3.x only :)

kevinSuttle commented 9 years ago

thanks for the quick sidebar @dougwilson :)

ghost commented 7 years ago

It will save one line of code, a brain thought and a calorie burned from moving my fingers. I couldn't agree more. +1

ciscoo commented 7 years ago

What was the final reasoning for including body-parser as a dependency for Express? This issue has quite a bit of feedback, but it has been quiet for 8 months so I'm curious as to what lead you to include it in Express.

naholyr commented 6 years ago

Remember why middlewares had been removed in the first place: when body-parser will receive a major bump, you'll need to bump express's major too. Do we really want to go toward this situation? Or the bundled express.json may fork or never get upgraded (or very later)?

I think it's worth answering just those questions before going further. 'cause at this point I don't see any reason not including session or morgan, which are both as often used as body-parser. I think this is a dangerous path ;)

dmwelch commented 6 years ago

Hi all, got here from a Stackoverflow post. According to the migration docs, I should still be using body-parser... or not... ??? https://expressjs.com/en/guide/migrating-4.html