Preserve data on get without dependency on server header #146

Merged
merged 5 commits into from May 21, 2012

Conversation

Projects
None yet
4 participants
@anttimattila
Contributor

anttimattila commented May 21, 2012

When server doesn't return url in X-PJAX-URL header many tests fail. Mainly due to data parameters not being added to url on GET requests.

This fixes all but redirection and reduces dependency on the response header.

anttimattila added some commits May 21, 2012

Test with equal() instead of ok()
This way test is more accurate and when it fails you can see why.
jquery.pjax.js
+ a.href = url
+ return a.href
+ }
+ options.requestUrl = qualifyURL(settings.url)

This comment has been minimized.

@josh

josh May 21, 2012

Contributor

Theres a parseURL helper already. parseURL(settings.url).href should work.

I think we should avoid setting requestUrl on options, maybe just make a new local for it.

@josh

josh May 21, 2012

Contributor

Theres a parseURL helper already. parseURL(settings.url).href should work.

I think we should avoid setting requestUrl on options, maybe just make a new local for it.

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh May 21, 2012

Contributor

Otherwise, I dig it.

Contributor

josh commented May 21, 2012

Otherwise, I dig it.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot May 21, 2012

This pull request passes (merged ed34151 into 73dd1ab).

This pull request passes (merged ed34151 into 73dd1ab).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot May 21, 2012

This pull request passes (merged 1185f50 into 73dd1ab).

This pull request passes (merged 1185f50 into 73dd1ab).

jquery.pjax.js
@@ -165,6 +165,8 @@ var pjax = $.pjax = function( options ) {
if (!fire('pjax:beforeSend', [xhr, settings]))
return false
+
+ pjax.requestUrl = parseURL(settings.url).href

This comment has been minimized.

@josh

josh May 21, 2012

Contributor

Maybe just a local? var requestUrl inside the pjax function scope.

@josh

josh May 21, 2012

Contributor

Maybe just a local? var requestUrl inside the pjax function scope.

This comment has been minimized.

@anttimattila

anttimattila May 21, 2012

Contributor

Isn't it used outside the pjax function scope:

function extractContainer(data, xhr, options) {
var obj = {}

// Prefer X-PJAX-URL header if it was set, otherwise fallback to
// using the original requested url.
obj.url = stripPjaxParam(xhr.getResponseHeader('X-PJAX-URL') || pjax.requestUrl)
...

@anttimattila

anttimattila May 21, 2012

Contributor

Isn't it used outside the pjax function scope:

function extractContainer(data, xhr, options) {
var obj = {}

// Prefer X-PJAX-URL header if it was set, otherwise fallback to
// using the original requested url.
obj.url = stripPjaxParam(xhr.getResponseHeader('X-PJAX-URL') || pjax.requestUrl)
...

This comment has been minimized.

@josh

josh May 21, 2012

Contributor

Ah, sorry its hard to tell from the diff. I guess the original way assigning it to options make sense then. Sorry for the confusion.

@josh

josh May 21, 2012

Contributor

Ah, sorry its hard to tell from the diff. I guess the original way assigning it to options make sense then. Sorry for the confusion.

This comment has been minimized.

@anttimattila

anttimattila May 21, 2012

Contributor

No problem. I changed it back.

@anttimattila

anttimattila May 21, 2012

Contributor

No problem. I changed it back.

@anttimattila

This comment has been minimized.

Show comment
Hide comment
@anttimattila

anttimattila May 21, 2012

Contributor

Not sure what you mean by local. I moved the requestUrl property from options to pjax, is that what you ment?
I made the changes and pushed them to the branch, apparently they also update this pull request.

Contributor

anttimattila commented May 21, 2012

Not sure what you mean by local. I moved the requestUrl property from options to pjax, is that what you ment?
I made the changes and pushed them to the branch, apparently they also update this pull request.

@josh

This comment has been minimized.

Show comment
Hide comment
@josh

josh May 21, 2012

Contributor

Just a local variable: var requestUrl.

Yeah, just keep pushing to the same branch. No need for another pull.

Contributor

josh commented May 21, 2012

Just a local variable: var requestUrl.

Yeah, just keep pushing to the same branch. No need for another pull.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot May 21, 2012

This pull request passes (merged 65f8c7c into 73dd1ab).

This pull request passes (merged 65f8c7c into 73dd1ab).

josh added a commit that referenced this pull request May 21, 2012

Merge pull request #146 from anttimattila/preserve-data-on-GET
Preserve data on get without dependency on server header

@josh josh merged commit e0d306d into defunkt:master May 21, 2012

@stephanfowler

This comment has been minimized.

Show comment
Hide comment
@stephanfowler

stephanfowler Jun 22, 2012

This commit seems to have caused an change in behaviour. Previously, this:
$.pjax({
url: 'http://example.com'.
data: { foo: 'bar' },
etc..
});

...would cause a pushState of this url:
http://example.com

...whereas after this commit, it causes a pushState of the url:
http://example.com?foo=bar

Any idea how force the former behaviour?

This commit seems to have caused an change in behaviour. Previously, this:
$.pjax({
url: 'http://example.com'.
data: { foo: 'bar' },
etc..
});

...would cause a pushState of this url:
http://example.com

...whereas after this commit, it causes a pushState of the url:
http://example.com?foo=bar

Any idea how force the former behaviour?

This comment has been minimized.

Show comment
Hide comment
@anttimattila

anttimattila Jun 23, 2012

Contributor

I haven't looked into this yet to see what's going on, but if this commit broke something, I'd just revert it.

Contributor

anttimattila replied Jun 23, 2012

I haven't looked into this yet to see what's going on, but if this commit broke something, I'd just revert it.

This comment has been minimized.

Show comment
Hide comment
@stephanfowler

stephanfowler Jun 23, 2012

Yep, did that. Thanks.

Yep, did that. Thanks.

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