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

#113 Build URL to determine if request is cached #297

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

Conversation

inkstak
Copy link

@inkstak inkstak commented May 9, 2016

In response to #113, I added buildUrlfrom angular to determine if request is cached or not.

var cache,
defaultCache = $cacheFactory.get('$http'),
defaults = $httpProvider.defaults,
url = buildUrl(config.url, config.paramSerializer(config.params));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Single variable declaration per line please

@faceleg
Copy link
Collaborator

faceleg commented May 9, 2016

Needs tests to support new feature

@inkstak
Copy link
Author

inkstak commented May 11, 2016

Updated, with tests cover.

buildUrl is simple to copy since angular 1.4, because it relays on $httpParamSerializer.
Before that, it depends on too many private API, that's why it'll work only with angular 1.4 and later.


expect(cfpLoadingBar.status()).toBe 0
$timeout.flush()
$timeout.flush()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are 2 timeout flushes necessary?

Copy link
Author

Choose a reason for hiding this comment

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

Good question.
I must admit that I stupidly copied the structure from existing tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've got some really odd looking stuff in some of my tests, I wasn't criticising you.

Do the tests work if you remove one of them?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for the delay.
I've just tried removing one flush, but both are required to pass the tests (mine and yours).
I guess that two are enough to complete timeouts in _set and _complete.

FYI, I just pushed source code changes. Not builds. Do you need them now ?

@faceleg
Copy link
Collaborator

faceleg commented May 12, 2016

@chieffancypants over to you

@chogarcia
Copy link

+1!

@faceleg
Copy link
Collaborator

faceleg commented Jul 20, 2016

@chieffancypants needs your approval

@chieffancypants
Copy link
Owner

Sorry to be so late to the party. For clarification, does this feature simply not work with angular 1.4, or does it break the entire library's compatibility with earlier versions of angular?

@inkstak
Copy link
Author

inkstak commented Jul 22, 2016

The feature is available with Angular 1.4 and later (which implements $httpParamSerializer)
It doesn't work with earlier versions, but doesn't break anything.

@inkstak
Copy link
Author

inkstak commented Oct 6, 2016

Hi @chieffancypants
Any news about merging ?

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

Successfully merging this pull request may close these issues.

4 participants