[codeface] Re: [PATCH 1/9] upgrade to latest nodejs changes.
- From: Mitchell Joblin <joblin.m@xxxxxxxxx>
- To: codeface@xxxxxxxxxxxxx
- Date: Wed, 26 Nov 2014 15:11:29 +0100
On Wed, Nov 26, 2014 at 3:01 PM, Matthias Dittrich <matthi.d@xxxxxxxxxxxxxx>
wrote:
> On 26.11.2014 14:39, Mitchell Joblin wrote:
>>
>> 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.
>
> That's the one from the pull request and it's the one I'm talking about:
>
https://github.com/matthid/codeface/commit/d1da972b87c18ea10f3014c5c4a90e31bbc1fd48
.
> You have to watch closely, it looks like I add bodyparser but you can see
in
> the deleted lines that it was used before. It's just that nodejs moved it
> out of the core distribution (so the old code will not work anymore).
> Does that clear everything up? Can you try what I suggested in my last
> email?
It is very clear that body-parser was used before your additions. The issue
is about the new calls that are added by your changes. For example
"bodyParser.urlencoded()" perhaps you should consult this resource for fix (
http://stackoverflow.com/questions/25471856/express-throws-error-as-body-parser-deprecated-undefined-extended
).
--Mitchell
> -- Matthias
>
>>
>> --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:
