lisaogren / axios-cache-adapter

Caching adapter for axios. Store request results in a configurable store to prevent unneeded network requests.
MIT License
726 stars 107 forks source link

Replace expensive lodash calls with native functions (cont. #115) #132

Closed rbeer closed 4 years ago

rbeer commented 4 years ago

Issue: Lodash dependency making up >30% of library's size. (#38) Solution:

  1. Replace lodash dependencies with own utilities library and native versions.
  2. Exclude core-js (babel) polyfills for widely available features (see this post for details)

With lodash (7d5efd9):

dist/
├── [193K | 197422]  cache.js
├── [ 45K |  46431]  cache.min.js
├── [256K | 262327]  cache.node.js
├── [ 57K |  57985]  cache.node.min.js

Without lodash and polyfills (6690cc6):

dist/
├── [167K | 171052]  cache.js
├── [ 41K |  42131]  cache.min.js
├── [179K | 183304]  cache.node.js
├── [ 45K |  45791]  cache.node.min.js
codecov[bot] commented 4 years ago

Codecov Report

Merging #132 into master will increase coverage by 0.66%. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #132      +/-   ##
========================================
+ Coverage   99.33%   100%   +0.66%     
========================================
  Files          11     12       +1     
  Lines         151    170      +19     
========================================
+ Hits          150    170      +20     
+ Misses          1      0       -1
Impacted Files Coverage Δ
src/memory.js 100% <100%> (ø) :arrow_up:
src/config.js 100% <100%> (ø) :arrow_up:
src/serialize.js 100% <100%> (ø) :arrow_up:
src/redis.js 100% <100%> (+8.33%) :arrow_up:
src/api.js 100% <100%> (ø) :arrow_up:
src/exclude.js 100% <100%> (ø) :arrow_up:
src/cache.js 100% <100%> (ø) :arrow_up:
src/utilities.js 100% <100%> (ø)

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 7d5efd9...7e55d60. Read the comment docs.

rbeer commented 4 years ago

I believe the last missing coverage reveals a race condition with the outer-scope firstItem and the fact that each store's entry is processed async. The tests pass, even when you reverse the order of setItem calls in the test, so that the expired item is second. Do I understand correctly that this line is to return the first item in order of expiration date?

I will open a separate issue for this with proper testing, if I find something.

ghost commented 4 years ago

❤️

Overall very nice work @rbeer! I need to reed these modifications a bit more closely so I can understand everything correctly, more specifically the replacement of _.merge with just using spread operator. I'll do this ASAP.

Very nice getting the coverage up on src/redis.js btw!

ghost commented 4 years ago

I believe the last missing coverage reveals a race condition with the outer-scope firstItem and the fact that each store's entry is processed async. The tests pass, even when you Do I understand correctly that this line is to return the first item in order of expiration date?

I will open a separate issue for this with proper testing.

Yes you are correct about that line of code. If you fix that in another PR it'll be awesome 🎉

AndrewCraswell commented 4 years ago

Hey @RasCarlito, when you get this merged if you have time, can you take a look at this PR? https://github.com/RasCarlito/axios-cache-adapter/pull/114

These two are the ones my team is hoping to get in.

AndrewCraswell commented 4 years ago

@rbeer, now that the issue has been closed you can claim the bounty :)

@RasCarlito thank you so much for taking the time to get these merged!