rbren / rss-parser

A lightweight RSS parser, for Node and the browser
MIT License
1.38k stars 209 forks source link

use fetch api instead of http/https packages #228

Closed matiasfha closed 1 year ago

matiasfha commented 2 years ago

The idea is to remove the usage of http and https package that are node dependent and instead use isomorphic-fetch to allow the parser to work on edge environments where the node packages can't run.

eviltik commented 2 years ago

+1

twhite96 commented 2 years ago

Anyone going to merge this?

rbren commented 1 year ago

I've tried to keep the dependencies here down to a minimum--especially important for running in the browser. I'm hesitant to include this.

I would encourage you to use fetch to get the RSS XML, and then use this package's parseString--that should get around the issue.

matiasfha commented 1 year ago

Isn’t fetch available on node, browser and edge runtimes now? That means that i can get rid of isomorphic-fetch

rbren commented 1 year ago

Hmm it does seem to have pretty solid browser support according to https://caniuse.com/fetch

I wonder if we can:

WDYT?

ghost commented 1 year ago

I tried to do this, and got:

./node_modules/rss-parser/lib/parser.js:6:12-26 - Error: Module not found: Error: Can't resolve 'url' in '\source\repos\MAMobile\node_modules\rss-parser\lib'

Here is my code:

parseUrlWrapper()
  {
    return new Promise<Parser.Output<Parser.Item>>((resolve,reject)=> {
      let p : Parser = new Parser({
        customFields: {
          item: ['image']
        }
    });
    fetch(this.targetUrl).then(response => {
        response.json().then(data => {
          p.parseString(data, function(err : Error, feed: Parser.Output<Parser.Item>) {
            if (err) {
              reject(err);
            } else {
              resolve(feed);
            }
          });
        });
      });
    });
  }
ghost commented 1 year ago

It looks like the pull request removed a lot of these dependencies - I had done my own pull request on this branch, and was using it unmerged - it looks like I'll need to go back to that since it still depends upon URL, Https, etc.

ghost commented 1 year ago

This proved to be correct, it is now working. the code above seems to work based on the advice given!