Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promise based filter function #89

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

koriwi
Copy link

@koriwi koriwi commented Dec 15, 2018

Needed this functionality really bad. So here it is.

@codecov
Copy link

codecov bot commented Dec 15, 2018

Codecov Report

Merging #89 into master will increase coverage by 0.81%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage   83.47%   84.29%   +0.81%     
==========================================
  Files           5        5              
  Lines         115      121       +6     
  Branches       38       40       +2     
==========================================
+ Hits           96      102       +6     
  Misses          7        7              
  Partials       12       12
Impacted Files Coverage Δ
src/index.ts 79.48% <100%> (+1.7%) ⬆️

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 63651d3...2f7d270. Read the comment docs.

@championswimmer
Copy link
Owner

okay, looks good, but the reason for this ?

also, you should add a test that covers the promise based filtering, so that the coverage doesn't go down.

@koriwi
Copy link
Author

koriwi commented Dec 15, 2018

Will take care about this tomorrow.

Why? To be able to use something like this
https://codesandbox.io/s/24vp95x72p

@cedricmay
Copy link

I could imaging using promises for a debounce like feature if the state is updating too fast and getting bigger, right? That might bring some performance improvements especially on mobile devices.

@koriwi
Copy link
Author

koriwi commented Dec 17, 2018

@championswimmer i added a test. Had to use a timeout because to wait for storage change.
Would be nice if you could merge/publish in a timely manner, because deployment for me with github is blocked because of an npm bug with the prepare script.

@Juice10
Copy link

Juice10 commented Apr 1, 2021

Any chance we could get this merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants