os-js / osjs-server

OS.js Server Module
https://manual.os-js.org
Other
19 stars 21 forks source link

unable to parse POST call body content #1

Closed aherreraGH closed 6 years ago

aherreraGH commented 6 years ago

Adding the body-parser before my own app.post() call got the data to appear:

var bodyParser = require('body-parser');
instance.app.use(bodyParser.urlencoded());
instance.app.post('/proxy', (req, res, next) => {

the req.body was always undefined until I added the body-parser to my code block

andersevenrud commented 6 years ago

I'm not quite sure why the .json() method from body-parser does not catch this case, but I can reproduce this issue locally. Application does not have this issue though, so I'm kinda scratching my head on this one. There should be no racing-condition in regards of this because when you do a app.{method}() it does not get called until it is all settled.

However, I don't see any reason not to include .urlencoded() in any case to the main server code. I'll add this tomorrow and publish when done. Getting late here :)

andersevenrud commented 6 years ago

I figured out that's it actually is a race condition. Since the body-parser is applied in a service provider, whenever you do an {express}.{method} in the server bootstrapping script the .json() middleware has not been applied yet.

I tested doing the same procedure in a service provider, and was unable to reproduce the issue.

So, I'll be adding an event that you can use for this:

instance.on('init', app => {
  app.post('/proxy', (req, res) => console.log(res.body));
});

I'll also add the urlencoded stuff too though, because it's probably useful :)

andersevenrud commented 6 years ago

note to self and @aherreraGH

The recommended way to solve this is to change the core provider to load early, and make a custom service to add routes etc. This ensures that required middleware loads first.

instance.register(CoreServiceProvider, {before: true});
// Your provider module
const {ServiceProvider} = require('@osjs/common');

class MyProvider extends ServiceProvider {
  async init() {
    this.core.app.post('/proxy', (req, res) => console.log(req.body));
  }
}

module.exports = MyProvider;

// Bootstrap file
const MyProvider = require('my-package');
instance.register(MyProvider);

That said, I'll make an update that provides an event mentioned in the previous comment so you can add things to the bootstrap file as an alternative,

I'll come back to you tomorrow on this when it's done and help out :)

andersevenrud commented 6 years ago

I've just published a new version of @osjs/server which provides an event that you can use to get around this issue:

// src/server/index.js:

instance.on('osjs/core:started', () => {
  instance.app.post('/xxx', (req,res) => {
    // Now works
    console.log(req.body);
    res.json({});
  });
});

As mentioned in my previous comment, if you're going to add more custom things, a service provider will most likely be a better option to reduce clutter etc :)