2
\$\begingroup\$

Application Summary

I received a call from a client asking for a "simple app" that notified him via text message whenever a "Jeep Wrangler" is posted to Facebook Marketplace. It sounded simple enough, so I took the gig. I figured I would leverage FB's Graph API or possibly simply set up a filter in his account, or something along those lines.

It wasn't long until I faced reality. This was going to be harder than I thought.

I ended up deciding to write a screen scraper. I chose NodeJS, Express, and Puppeteer to do this.

End User UI

App

A JSON Failure

When I was almost done with the app, I realized I could not use JSON as my data storage, as I intended. Heroku apparently uses Dyno's that sleep, and data does not persist. I don't fully understand it, but I had to take a different approach.

Finally...

Well, this is what I came up with so far. I am pretty much done, I just need to fix my HTML page to use <% %> tags to display the data.

const puppeteer = require('puppeteer');
const jsonfile  = require("jsonfile");
const _         = require("lodash");
var mysql       = require('mysql');

var browser;
var page;

// Connect to database
var pool  = mysql.createPool({
  connectionLimit : 10,
  host            : 'localhost',
  user            : 'root',
  password        : '',
  database        : 'marketplace'
});

global.pool = pool;

// Gets current items  Search Results
const getItems = async searchTerm => {

  browser = await puppeteer.launch({
    headless: true,
    timeout: 0,
    args: ["--no-sandbox"]
  });

  page = await browser.newPage();
  await page.goto(`https://facebook.com/marketplace/tampa/search/?query=${encodeURI(searchTerm)}&sort=created_date_descending&exact=true`);
  await autoScroll(page);

  const itemList = await page.waitForSelector('div > div > span > div > a[tabindex="0"]')
    .then(() => page.evaluate(() => {

      const itemArray = [];
      const itemNodeList = document.querySelectorAll('div > div > span > div > a[tabindex="0"]');
      
      itemNodeList.forEach(item => {

        const itemTitle = item.innerText;
        const itemURL = item.getAttribute('href');
        const itemImg = item.querySelector('div > div > span > div > a > div > div > div > div > div > div > img').getAttribute('src');

        var obj = ['price', 'title', 'location', 'miles',
            ...itemTitle.split(/\n/)
          ]
          .reduce((a, c, i, t) => {
            if (i < 4) a[c] = t[i + 4]
            return a
          }, {});

        obj.imgUrl  = itemImg;
        obj.itemURL = itemURL;

        itemArray.push(obj);
      });

      return itemArray;

    }))
    .catch(() => console.log("Selector error."));

  return itemList;

}


const initScraper = async () => {

  var finalArray    = [];
  var currentItems  = [];
  var previousItems = [];


  // Scrape Page - Get New Items
  currentItems = await getItems('Jeep Wrangler');
  // Save Data: previousJeeps
  const insertCurrentSuccess = await saveToDatabase('previousJeeps',currentItems);

  allDone();

  // Get Previous Items From Database
  previousItems = await getPreviousItems();

  // Get Differences
  finalArray = _.difference(currentItems, previousItems);
  //console.log(finalArray);
 
  // Save Data: newJeeps
  const insertNewSuccess = await saveToDatabase('newJeeps',finalArray);

  // If New Items, Notify User
  if (!_.isEqual(currentItems, previousItems)) {
    changed = true;
    const page2 = await browser.newPage();
    await page2.goto(`http://john.mail.com/mail.php`);
    console.log("changed");
  }

  // Let us know when done
  console.log("done");

}

initScraper();

const allDone = async function(){
  console.log("All done");
  //process.exit();
}

//----------------------------------------------------
// This function loads the entire search results from
// last time - so it can be compared against the
// new search results.about_content
//----------------------------------------------------
const getPreviousItems = async function () {  
    pool.query("SELECT * FROM previousJeeps", function (err, result, fields) {
      if (err){
        console.log(err);
        // Redirect to error page    
      } else {
        return result;  
      }
      
    });
}


// Save Data
const saveToDatabase = async function (tblName, results) {

  /*
 results.forEach(element => {

    var sql = "";
    var title = title.replace(/'/g, "\\'");;
    
    var location= location.replace(/'/g, "\\'");;;
    var miles= miles.replace(/'/g, "\\'");;;
    var imgUrl= imgUrl.replace(/'/g, "\\'");;;
    var itemURL= itemURL.replace(/'/g, "\\'");;;
    

     sql = "INSERT INTO  " + tblName +
           "SET (title, price, location, miles, imgUrl, itemURL, status, is_deleted)" +
           "VALUES (" +
           "'${title}', '${element.price}', '${location}', '${miles}', '${imgUrl}', '${itemURL}', 1, 0" +
           ")";

      pool.query(sql, function (err, rows, fields) {
            if (err) throw err;
      });

  }) 
  
 */
  return true;

}


// This takes care of the auto scrolling problem
async function autoScroll(page) {
  await page.evaluate(async () => {
    await new Promise(resolve => {
      var totalHeight = 0;
      var distance = 100;
      var timer = setInterval(() => {
        var scrollHeight = document.body.scrollHeight;
        window.scrollBy(0, distance);
        totalHeight += distance;

        if (totalHeight >= scrollHeight || scrollHeight > 9000) {
          clearInterval(timer);
          resolve();
        }
      }, 100);
    });
  });
}
//----------------------------------------------------

Any criticism , good or bad, welcome. I wonder if this could have been done better, more efficient, etc.

\$\endgroup\$
4
  • \$\begingroup\$ I can see quite a few logic problems; the script will not work as it should on a number of crucial steps. You might want to try running the script and debugging to get it at least in mostly working order first, per CR rules \$\endgroup\$ – CertainPerformance May 9 '20 at 6:06
  • \$\begingroup\$ Thanks for looking. It actually works great. The code above is an early version of V2 which is where I went from JSON to MySQL. I should comment out the DB stuff, just to get some feedback on the architecture alone. But I am curious, what logic issues do you see? \$\endgroup\$ – John S. May 9 '20 at 6:18
  • \$\begingroup\$ saveToDatabase doesn't return anything, nor does getPreviousItems, previousItems will always be undefined inside initScraper, so the finalArray = _.difference(currentItems, previousItems); and other stuff which depends on finalArray and previousItems won't work \$\endgroup\$ – CertainPerformance May 9 '20 at 6:29
  • \$\begingroup\$ @certain, you are correct, and I am new to this forum. I now know to post finalized version of my code. What I was looking for was some advice on the approach as a whole. It just seems like there is a better way than: 1. Scrape, 2. Open last scrape file. 3. Compare. 4. Save differences back to file (in #2). 5. If any changes, email user. 6. Repeat. It just seems like a lot of work. Any suggestions? I foresee about one to two new items a day, in case that affects your suggestion. Thank you for looking. I will update the code when I get to office. Have a great night. Thanks again \$\endgroup\$ – John S. May 10 '20 at 5:57
2
\$\begingroup\$

One thing that stands out to me is the database itself. It's somewhat ugly for something that sounds like it only really needs to keep track of one thing: a unique identifier for each truck viewed so far (such as its page URL), if the client wants to be alerted on new postings, and not on edits to old postings. If it were me, I'd set the script up on my own server, which does have a persistent file system, and then it'd be trivial to save and retrieve the URLs seen so far with JSON.stringify/JSON.parse with a tiny local file. If that's not possible, you can still make things simpler by saving just the URLs of each truck into the database, then checking whether the URL of a truck found on the page exists in the database yet or not.

It's pretty much never a good idea to directly concatenate input to construct the SQL query string:

sql = "INSERT INTO  " + tblName +
      "SET (title, price, location, miles, imgUrl, itemURL, status, is_deleted)" +
      "VALUES (" +
      "'${title}', '${element.price}', '${location}', '${miles}', '${imgUrl}', '${itemURL}', 1, 0" +
      ")";

It's not only inelegant, when done wrong, it can lead to (inadvertent) SQL injection and other troubles. Consider using parameterized queries instead.

You might also consider using Redis instead of a database, I think it might be a slightly better choice, especially if you just need to store an array of URLS. I believe you could do something like:

// Retrieve all elements in "truckUrls" list
// lrange: retrieves all elements of list
// Use a Set for less computational complexity
const existingUrls = new Set(await lrange('truckUrls', 0, -1));

const currentlyDisplayedItems = await getItems();

const newItems = currentlyDisplayedItems.filter(({ itemURL }) => !existingUrls.has(itemURL));
if (newItems.length > 0) {
  // Save new URLs:
  // rpush: pushes elements to a list
  await rpush('truckUrls', ...newItems.map(({ itemURL }) => itemURL));
  // Then notify user with items from the newItems object here
}
// Done

where lrange and push, Redis methods have been promisified. (By default, they use callbacks, just like your existing pool.query.) To convert a callback API to a Promise, either use util.promisify (recommended) or do it manually. (Your current getPreviousItems and saveToDatabase are not promisified, so they resolve immediately, rather than when the action is complete, and don't resolve to anything.)

In your initScraper function, there's no need to assign to a variable that isn't going to be read before it's reassigned:

var currentItems  = [];
// ...


// Scrape Page - Get New Items
currentItems = await getItems('Jeep Wrangler');

Better to declare the variable only after the value to assign to it is retrieved:

const currentItems = await getItems('Jeep Wrangler');

Note the use of const. You're sometimes declaring variables with var, and sometimes with const. If you're writing in ES2015+ syntax (which you are, and should be), you should always use const to declare variables: var has too many gotchas to be worth using, and is less readable than const (since with const, you know that a variable is never going to be reassigned). If a variable must be reassigned, declare it with let.

Make sure to handle errors - unhandled Promise rejections are deprecated and in the future will cause the Node process to terminate. Best place to handle them would probably be at the entry point, the initScraper call:

initScraper()
  .catch((err) => {
    // handle errors
    // add to a logfile?
  });

I think the only thing left to look at is the getItems function.

The reassignment of the global browser variable used both in getItems and initScraper is somewhat smelly:

browser = await puppeteer.launch({
  headless: true,
  timeout: 0,
  args: ["--no-sandbox"]
});

Consider constructing the browser in initScraper instead, and then pass it to getItems - that'll let you declare it with const, and avoid an unnecessary global variable. page doesn't need to be global either - it's only used inside getItems, so feel free to declare it with const inside.

Since you don't need to use the page.waitForSelector result directly, and since you're using await already (which is good!), you might use await page.waitForSelector and separately do const itemList = await page.evaluate. This also less you get rid of a layer of bracket nesting.

You have a couple of very specific selectors. If they work, that's fine, but the slightest tweak to Facebook's HTML will cause your script to break. You might consider using the descendant selector instead when possible, and with something more specific than tag names. For example, it would be great if you could replace div > div > span > div > a[tabindex="0"] with a selector similar to .listContainer a[tabindex="0"] where listContainer is a class on an ancestor element - look through the DOM to see if something like that is possible. (Rather than repeating this selector twice, save it in a variable first, then reference that variable.) Same thing for itemImg's selector - you might be able to replace

item.querySelector('div > div > span > div > a > div > div > div > div > div > div > img')

with

item.querySelector('img[src]')

It's best to avoid .innerText unless you're deliberately looking to invoke its strange text styling rules. See if you can use textContent instead, which is the standard method.

If you're iterating over all elements of an array to construct a new one, it's more appropriate to use Array.prototype.map (from which you can return the item for the new array) than to use forEach and push. (See below for example.)

The reduce there is really weird. If you have an array of values that you want to put into an object with particular key names, using a plain object literal by destructuring the split call would make more sense (see below for example).

In full, getItems can be made to look something like the following:

const getItems = async (searchTerm, browser) => {
  const page = await browser.newPage();
  await page.goto(`https://facebook.com/marketplace/tampa/search/?query=${encodeURI(searchTerm)}&sort=created_date_descending&exact=true`);
  await autoScroll(page);
  const itemSelector = 'div > div > span > div > a[tabindex="0"]';
  await page.waitForSelector(itemSelector);
  return page.evaluate(() => {
    return [...document.querySelectorAll(itemSelector)]
      .map((item) => {
        const itemTitle = item.textContent;
        const itemURL = item.href;
        const imgUrl = item.querySelector('img[src]').src;
        const [price, title, location, miles] = itemTitle.split(/\n/);
        return { price, title, location, miles, imgUrl, itemURL };
      });
  });
};
\$\endgroup\$
1
  • \$\begingroup\$ Wow thank you so much! I can't wait to dig in and try every suggestion. Before I read this answer from you, I made some good progress ( on the same path, however ). The code is here: gist.github.com/johnsdeveloper/b856195b0d23efd219e18db2da5ff4fc , and if I could get the darn insert to work, I think I would be in good shape, Although I agree, your re-write is definitely in order. If the code in my latest gist worked, I would set up the cron for every 10 mins. It would simply write the new vehicles to a table the front end would grab. Looking at it now, though, needs ALOT of work. \$\endgroup\$ – John S. May 11 '20 at 2:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.