Codecov Report

Merging #142 (7363d52) into master (cf898c2) will decrease coverage by 1.51%. The diff coverage is 100.00%.

@@             Coverage Diff             @@
##            master     #142      +/-   ##
===========================================
- Coverage   100.00%   98.48%   -1.52%     
===========================================
  Files            3        3              
  Lines           40       66      +26     
  Branches         4       26      +22     
===========================================
+ Hits            40       65      +25     
- Misses           0        1       +1

Impacted Files	Coverage Δ
src/functions/utils/fetch.js	`98.43% <100.00%> (-1.57%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cf898c2...7363d52. Read the comment docs.

omarryhan commented 4 years ago

I must say that the extra export I added will significantly increase the bundle size because it will ship Cheerio in the bundle, which is a bad idea because rarely does anyone scrape on the browser. I can remove the export, but then we won't be able to import it on server-side because right now Rollup isn't configured to do that. To solve that we can assign multiple outputs in rollup.config.js like shown here. This would lets us remove the extra export I added in index.js.

After:

  {
    input: 'src/index.js',
    external,
    output: [
      { file: pkg.main, format: 'cjs' },
      { file: pkg.module, format: 'es' },
    ],
    plugins: [
      json(),
      resolvePlugin,
      babelPlugin,
      process.env.NODE_ENV === 'production' && terser(),
    ],
  },

we'll add:

  {
    input: 'src/functions/utils/fetch.js',
    output: {
        file: 'dist/scrape.js',
        format: 'cjs'
    },
    plugins: [
      resolvePlugin,
      babelPlugin,
    ],
  },

That way, if we want to import the scraping functions in Node.js, we do the following:

import * as scrapingFunctions from '@huchenme/github-trending/scrape';

async () => {
    await scrapingFunctions.scrapeRepositories();
}()

What do you think?

omarryhan commented 4 years ago

I just separated both the scraping code and the code that calls the API so that the browser bundle doesn't grow. I also addressed your comments about the code style. I added some other changes to accommodate the splitting of the code. Let me know if there's anything that's not clear.

omarryhan commented 4 years ago

Right now, Rollup should be processing 4 files instead of just 1.

index.js + scrape.js + index.d.ts + scrape.d.ts

You can find more info about my previous commit in the commit's message.

huchenme / github-trending-api

Expose the scraping functions for usage in node.js #142

Codecov Report