search

Tjatse / node-readability

Scrape/Crawl article from any site automatically. Make any web page readable, no matter Chinese or English.
341 stars 36 forks source link

Consider adding promises #33

Closed midudev closed 8 years ago

midudev commented 8 years ago

Not a issue but a feature. I think that would be great having promises on the package as later and LTS Nodejs versions have official support.

read(url).then((art, options, res) => {
  console.log(art.body)
}).catch((err) => {
  console.log(err)
})
Tjatse commented 8 years ago

It's a very wise proposal as usual:) Thanks!

Tjatse commented 8 years ago

Hey, @miduga , please try npm install read-art@0.4.9-beta, e.g.:

read('<body><p>Hello, read-art</p></body>')
  .then(function (art, options, httpResponse) {
    // TODO:
  }, function (err) {
    // Handle error.
  })
  .catch(function (err) {
    // Catch exception.
  })
midudev commented 8 years ago

Will do. It looks fantastic. :)

midudev commented 8 years ago

First of all, as always, amazing work. Thanks. 👍

I've tried the last beta and I see that the options and response parameters are undefined. While art has options and serverResponse properties with the info. Is that intended or a bug? 👍 If is intended, then options and response on the callback are not needed.

  read(url)
    .then((art, options, response) => {
      console.log(art) // -> fine object with all the info
      console.log(options) // -> undefined
      console.log(response) // -> undefined
      done()
    })
    .catch(done)
Tjatse commented 8 years ago

It's totally intended, Cause only the first parameter will be treated as resolution value in the promise constructor, i.e.: Promise.resolve(value), it does not support multi arguments

midudev commented 8 years ago

Perfect. So, I would change the new documentation to remove those params and keep it with the useful param. 👍 Good job!

  read(url)
    .then((article) => {
      console.log(article) // -> object with all the info
    })
    .catch((err) => {
       // manage error
    })
midudev commented 8 years ago

I've modified the documentation in a Pull Request. ;)