CODEWITHSUNDEEP
javascriptnode.js
Otavio Bonder
Otavio Bonder

Reputation: 1999

NodeJS: TypeError: Cannot read property 'json' of undefined

I'm creating a CronJob that calls an API and store its response in the database:

const CronJob = require("cron").CronJob;
const btc_price_ticker = require("../../controllers/BtcExchange/Ticker");
const currency = require("../../controllers/Currencies/CurrenciesController");

module.exports = new CronJob("* * * * * *", async function() {
  const {
    ticker: { sell }
  } = await btc_price_ticker.getBtcPrice();
  currency
    .update({
      params: {
        id: "5cbdf078f5bcec257fcec792"
      },
      body: {
        exchange_rate: sell,
        lastUpdate: Date.now()
      }
    })
    .catch(error => console.log(error));
});

It works fine, however I receive a TypeError: Cannot read property 'json' of undefined

I'm using the same function to update the database I use when updating by my API:

module.exports = {
  async update(req, res) {
    const currency = await Currency.findByIdAndUpdate(req.params.id, req.body, {
      new: true
    });
    return res.json(currency);
  }
};

The TypeError happens in the return res.json(currency), and it only happens when it's called by the CronJob. When I put new information by API, it doesn't show any error.

I think it happens because when I call the function in CronJob, I just pass the req by parameter, but I don't know how to solve it. What am I supposed to do?

Thanks in advance!

Upvotes: 8

Views: 13299

Answers (3)

Nir Alfasi
Nir Alfasi

Reputation: 53525

There's a famous saying which says that you can solve almost any problem in CS by adding another layer of indirection. This is one of those cases:

Instead of of declaring your module as:

module.exports = {
  async update(req, res) {
    const currency = await Currency.findByIdAndUpdate(req.params.id, req.body, {
      new: true
    });
    return res.json(currency);
  }
};

Separate the logic from the route-logic:

module.exports = {

  async getCurrency(id, params) {
      const currency = await Currency.findByIdAndUpdate(id, params, {
        new: true
      });
      return currency;
  }

  async update(req, res) {
    const currency = await getCurrency(req.params.id, req.body);
    return res.json(currency);
  }
};

Now the route can call update() and the cron-job can call getCurrency() directly.

Upvotes: 3

1565986223
1565986223

Reputation: 6718

It's because both req and res is undefined (or atleast not the proper request and response object) when the route is not requested by client (which is the case here).

Simplest solution I can think of is mock (or make an actual call) a client call in your cron job by using modules like axios, request, request-promise (with Promise wrapper on top of request) like:

const rp = require('request-promise')

module.exports = new CronJob("* * * * * *", async function() {
  try {
    const {
      ticker: { sell }
    } = await btc_price_ticker.getBtcPrice();
    // assuming your server is running in localhost, port 3000 and route is 'myapi/:id'
    const url = "http://localhost:3000/myapi/5cbdf078f5bcec257fcec792";
    const options = {
      method: 'GET',
      uri : url,
      json: true,
      body: {
        exchange_rate: sell,
        lastUpdate: Date.now()
      }
    }
    const response = await rp(url);
  } catch(error) {
    console.log(error);
  }
});

Since you can execute the db methods, a simpler alternative would be to directly execute Currency.findByIdAndUpdate in your cron job. I don't see any reason why you would want to call your route in your code.

Upvotes: -1

Otavio Bonder
Otavio Bonder

Reputation: 1999

Ok, maybe my approach isn't the best, but since I'm passing an undefined object to update(), and I don't need the response, I edited the update function:

async update(req, res) {
    const currency = await Currency.findByIdAndUpdate(req.params.id, req.body, {
      new: true
    });
    if (res !== undefined) {
      return res.json(currency);
    }
  }

So, since res is undefined, it doesn't return anything.

Upvotes: -1

Related Questions