[codeface] Re: [PATCH 1/9] upgrade to latest nodejs changes.
- From: Mitchell Joblin <joblin.m@xxxxxxxxx>
- To: codeface@xxxxxxxxxxxxx
- Date: Wed, 26 Nov 2014 14:39:00 +0100
On Wed, Nov 26, 2014 at 2:29 PM, Matthias Dittrich
<matthi.d@xxxxxxxxxxxxxx> wrote:
>
> On 26.11.2014 12:38, Mitchell Joblin wrote:
>
> On Tue, Nov 25, 2014 at 7:32 PM, Matthias Dittrich
> <matthi.d@xxxxxxxxxxxxxx> wrote:
>
> On 25.11.2014 16:20, Mitchell Joblin wrote:
>
> On Mon, Nov 17, 2014 at 8:49 PM, Wolfgang Mauerer <wm@xxxxxxxxxxxxxxxx>
> wrote:
>
> Am 17/11/2014 16:22, schrieb Matthias Dittrich:
>
> On 17.11.2014 16:10, Wolfgang Mauerer wrote:
>
> Am 17/10/2014 15:14, schrieb Matthias Dittrich:
>
> This change makes id_service.js compatible with the latest nodejs
> versions.
>
> does "latest" version refer to development version, or the latest
> released version?
>
> I have installed net-libs/nodejs 0.10.21 on my gentoo (which are normaly
> quite up to date with their packages) machine, and that version needs
> that change...
>
> ok, so it's required for a released version.
>
> Signed-off-by: Matthias Dittrich <matthi.d@xxxxxxxxx>
> ---
> README.md | 2 +-
> id_service/id_service.js | 9 +++++----
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/README.md b/README.md
> index 4574d23..277a67a 100644
> --- a/README.md
> +++ b/README.md
> @@ -155,7 +155,7 @@ See `bugextractor/INSTALL` for all java-related
> details.
> * The ID service requires a few node.js packages. Install them by
> running
> - npm install addressparser express js-yaml mysql
> + npm install addressparser express js-yaml mysql body-parser
> in the `id_service` directory.
> diff --git a/id_service/id_service.js b/id_service/id_service.js
> index 2bb5afe..48bc7c1 100644
> --- a/id_service/id_service.js
> +++ b/id_service/id_service.js
> @@ -21,6 +21,7 @@ var mysql = require('mysql');
> var yaml = require("js-yaml");
> var logger = require('./logger');
> var addressparser = require("addressparser");
> +var bodyParser = require('body-parser');
> // get property file name
> var fileName = process.argv[2];
> @@ -63,10 +64,10 @@ var db_config = {
> };
> var pool = mysql.createPool(db_config)
> -app.configure(function() {
> - // used to parse JSON object given in the body request
> - app.use(express.bodyParser());
> -});
> +var env = process.env.NODE_ENV || 'development';
>
> ... here it seems you're referring to the development version.
>
> https://github.com/strongloop/express/wiki/Migrating-from-3.x-to-4.x
>
> But I suppose we will need body-parser when this version becomes
> a release? If yes, is there maybe a way to use something like
> a node.js version number?
>
> I'm not really into nodejs and how they handle releases, sorry.
> I can just say that I needed those changes because the old code would
> fail and I saw that this is the recommended change to do.
> If you don't need different environments I think it's fine to remove
> that if clause and keep the app.use call.
>
> We did not distinguish between development and production
> environments before. Even if we distinguished between development
> and production environment, why would it help to call the bodyParser()
> constructor only during development?
>
> Best regards, Wolfgang
>
> +if ('development' == env) {
> + app.use(bodyParser());
>
> At least according to
>
> http://stackoverflow.com/questions/24330014/bodyparser-is-deprecated-express-4,
> this call has been deprecated
> for many years. The github site you mention does also not say
> that you need to call bodyParser(). Can you please make sure
> a) why we need bodyParser(), and b) how it's used correctly, if we
> need it? Thanks!
>
> @Matthias, what's the status of this? I have merged your latest branch
> (locally) and am testing it out. When I start the id service I get the
> following
>
> Starting REST Service with logging level "info"
> info: Connecting to MySQL at codeface@localhost database codeface
> body-parser deprecated undefined extended: provide extended option
> id_service.js:68:20
>
> --Mitchell
>
> I do not see this on my system and I have no idea what that means. Is it at
> least working?
>
> It does not have any visible failures at this point. You added
> body-parser in one of you commits and it looks like one of the calls
> is already deprecated.
>
> No I did not add anything at all, codeface used bodyparser before. It's just
> that nodejs moved bodyparser out of their core distribution (if I understood
> that right). That is the reason why the previous code will not work on newer
> nodejs versions. Than I changed the code again in the latest rebase because,
> calling bodyparser() was deprecated (again I did not add anything!).
On your latest pull request there is a commit titled "Upgrade to
latest nodejs changes" and there is 4 deleted and 2 added lines to the
id_services.js file. This is the change I am talking about. Are you
saying this patch is not the right one? If so then please update the
pull request.
--Mitchell
>
> See
> http://stackoverflow.com/questions/24330014/bodyparser-is-deprecated-express-4
> If you want you can try and change the code to
>
> app.use(bodyParser.urlencoded({
> extended: true
> }));
>
> Can you test that code as I have no warnings on my system? If it works could
> you commit it on top of my changes, push it to github and send me the hash.
> This way I could cherry-pick the commit and sqash it in (if you want it that
> way; maybe even keep the separate commit?).
>
> It seems like they don't know what they want to do and deprecate things all
> the time, but at least it will now work on newer nodejs versions...
> I hope we can now just get over this change that doesn't actually do
> anything besides trying to make things work again :).
>
> --Matthias
>
> --Mitchell
>
> --Matthias
>
> Best regards, Wolfgang
>
> +}
> /**
> * Obtain a connection from the pool, check if it works, and then
> execute
>
>
>
Other related posts:
