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

Allow to use Promises on discard function #53

Merged
merged 5 commits into from
Oct 24, 2017

Conversation

piranna
Copy link

@piranna piranna commented Oct 18, 2017

This allow to return a Promise on the discard function so it's possible to decide asynchronously if a request should be discarded.

@piranna
Copy link
Author

piranna commented Oct 21, 2017

Any update on this? I need this for my work... :-/

@wacii
Copy link
Collaborator

wacii commented Oct 22, 2017

How would you use this? Do you have a sample discard() function?

@piranna
Copy link
Author

piranna commented Oct 22, 2017

How would you use this?

My use case is to check when I get a 401 Unauthorized error when doing a request because the access token got expired, so I need to request for a token refresh (that's an async operation) and if it went good then retry the unauthorized request with the new token header.

Do you have a sample discard() function?

import defaults_discard from '@redux-offline/redux-offline/lib/defaults/discard'

let refresh

function setRefresh(_refresh)
{
  refresh = _refresh
}

function discard(error, action, retries)
{
  if(error.status === 401) return refresh().then(() => false)

  return defaults_discard(error, action, retries)
}

Maybe should I to update the README.md docs?

@adrianicv
Copy link

I agree with @piranna, it would be awesome to have this functionality!

@piranna
Copy link
Author

piranna commented Oct 23, 2017

I've added tests and documentation for returning a Promise object from discard() function.

README.md Outdated
being processed, and a number representing how many times the action has been
retried. If the method returns `true`, the action will be discarded; `false`,
and it will be retried. The full signature of the method is `(error: any,
action: OfflineAction, retries: number) => boolean`. Alternatively, you can
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would update the signature and leave it at that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you means?

README.md Outdated
action: OfflineAction, retries: number) => boolean`. Alternatively, you can
return a Promise object that resolve to a boolean, allowing you to detect when
to discard asynchronously (for example, doing a request to a server to refresh a
token and try again).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your editor appears to have inserted line breaks here. It doesn't affect the resulting document, but I'd prefer to leave them out, just for consistency.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added them myself, I'm used to 80 columns also on Markdown files. I don't like to scroll horizontally and I would change the full file instead to adjust it, but if you preffer to have it the old way I can be able to change it.

@wacii
Copy link
Collaborator

wacii commented Oct 23, 2017

@piranna Thanks for the clarification. Other than some minor changes with the documentation, it looks good.

@sorodrigo is getting ready for a release, and then we are looking to merge back into the original repo, so I'm not sure when we'll get to this.

@sorodrigo
Copy link
Owner

I think its a good addition. Also, there a != somewhere near your changes, could replace it for a !==?

@sorodrigo
Copy link
Owner

I say we include this as a last addition before the next release

@wacii
Copy link
Collaborator

wacii commented Oct 23, 2017

The != was there before. And I'd be good with including it now.

@sorodrigo
Copy link
Owner

Yes I know haha it’s a style choice of the previous author. But I say if we are already updating this file why not change it. But its not THAT important.

@piranna
Copy link
Author

piranna commented Oct 24, 2017

I think its a good addition. Also, there a != somewhere near your changes, could replace it for a !==?

delay != null allow to delay to be both null and undefined. I think it's good to have it this way and only use === when you want explicitly check for exactly only one of them...

@sorodrigo
Copy link
Owner

Ok, just make the last changes @wacii suggested and we can merge

@piranna
Copy link
Author

piranna commented Oct 24, 2017

Ok, just make the last changes @wacii suggested and we can merge

The one of the splitted lines on the README.md file?

@piranna
Copy link
Author

piranna commented Oct 24, 2017

I have removed the splitlines on README.md. Is there anything more I need to do?

@sorodrigo
Copy link
Owner

Thanks!

@sorodrigo sorodrigo merged commit 0c2e1ce into sorodrigo:develop Oct 24, 2017
@piranna
Copy link
Author

piranna commented Oct 24, 2017

Thanks!

You are welcome! :-D Do you know when it could be published on npm? :-)

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