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

Feature/network first option #25

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

Conversation

mpatafio
Copy link

Hi @Exilz !

you lib is great!

I was trying to manage a real use case in the mobile world (and not only in it I guess): make a request perform a network first operation rather than check cache availability at the beginning of the flow. This will allow clients to try to refresh cache without altering the service definition, also because it wasn't possibile by defining any kind of middleware. This way clients are able to use the same service (then same cache) and by customising service options they can make the request behave accordingly to the above need.

Hope it makes sense.

Regards

@Exilz
Copy link
Owner

Exilz commented Apr 15, 2019

Hi,
thanks for the contribution.

This is something I thought about adding, too.

Could you make sure not to commit your .idea folder ? 😅 You can add it to the .gitignore as well.

@mpatafio
Copy link
Author

Removed idea files 👍 thanks

@mpatafio
Copy link
Author

mpatafio commented Apr 16, 2019

Hi @Exilz,

I've removed the stuff you suggested, then updated the Readme and bumped the minor version. The PR is ready to be merged if you want.

@mpatafio
Copy link
Author

mpatafio commented May 9, 2019

@Exilz up!

@Villar74
Copy link

Villar74 commented Jul 9, 2019

@Exilz up please)

@chicio
Copy link

chicio commented Jul 22, 2019

@Exilz up pleasssseeeeeeeee!!!

@Villar74
Copy link

don't leave uuuus XD @Exilz

@Villar74
Copy link

Villar74 commented Aug 7, 2019

@Exilz merge pls

.gitignore Outdated Show resolved Hide resolved
.watchmanconfig Outdated Show resolved Hide resolved
dist/drivers/sqlite.js Show resolved Hide resolved
dist/index.js Show resolved Hide resolved
dist/index.js Show resolved Hide resolved
dist/index.js Show resolved Hide resolved
dist/index.js Show resolved Hide resolved
dist/index.js Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@Exilz
Copy link
Owner

Exilz commented Aug 7, 2019

Thanks for the code review @Villar74 . I added some comments, too.
I wouldn't change anything on the generated javascript file. Let typescript do its thing.

Once you've made the requested changes, let's squash your commits before merging them.

@Villar74
Copy link

Villar74 commented Aug 7, 2019

Once you've made the requested changes, let's squash your commits before merging them.

@Exilz You talked to @mpatafio here?

@Exilz
Copy link
Owner

Exilz commented Aug 7, 2019

@Villar74 yes 😄

@mpatafio
Copy link
Author

mpatafio commented Aug 7, 2019

Hi guys!

Thank you for the comments! I’ll fix them as soon as I’ll come back from holiday (likely 21st of August)

@mpatafio
Copy link
Author

Hi guys! Thank you for the code review, I've resolved all the comments (apart the generated js files). Now I guess it can be merged. Just one thing for @Exilz: the image showing the flow of the library should be changed according to this new flow (conditioned by network first option), I changed the documentation, cannot do the same for such image. Thank you!

@Villar74
Copy link

@Exilz up

@mpatafio
Copy link
Author

are you still there guys?

@Villar74
Copy link

Villar74 commented Nov 5, 2019

@Exilz wake up pls, bro)

@Villar74
Copy link

Villar74 commented Dec 3, 2019

@Exilz Maxime, merge pls

Repository owner deleted a comment from Soumya6Tiwari Feb 23, 2024
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.

None yet

4 participants