Ajax: use anchor tag for parsing urls #1880

btoews · 2014-11-23T01:31:33Z

This PR changes the Ajax URL parsing from using a regex to using an anchor tag. This should be simpler and more reliable.

Fixes #1875
/cc @dmethvin

btoews · 2014-11-24T04:36:05Z

Lost some comments when force-pushing. I'm not sure if this is because I amended the commit rather than rebasing.

dmethvin · 2014-12-01T21:46:34Z

Ow, some thorny issues here. Unit tests fail because they assert that the URL has not been changed other than removing the hash. There are some other failures but I haven't looked into what's causing them.

The new behavior of changing the URL from relative to absolute in particular might be a problem. Can you run the unit tests locally and recommend a solution? We might, for example, leave the settings.url unchanged except for removing the hash, but keep the new scheme for our internal parsing?

btoews · 2014-12-01T21:53:06Z

I thought I was running CI locally, but apparently not. I'll try again.

btoews · 2014-12-02T19:34:10Z

I updated this PR to only use the new URL parsing for setting crossDomain. It's too bad to not be able to use this for the URL rewriting as well, but I understand that we want to minimize changes.

btoews · 2014-12-02T19:34:59Z

Also, I got CI running locally, and this fixes the failing tests.

dmethvin · 2014-12-02T19:54:09Z

Thanks @mastahyeti, yeah i wish we could clean up the URL but my gut feeling is that it would break a lot of code. @jaubourg could you take a look as well and give your thoughts? The new approach looks a lot cleaner in any case.

gibson042 · 2014-12-06T19:12:56Z

src/ajax.js

This should still be using s.url like the code it replaces.

btoews · 2014-12-09T17:01:44Z

I've addressed all the issues that were brought up in review. Is this good to go?

jaubourg · 2014-12-09T17:06:59Z

LGTM 👍

jaubourg · 2014-12-09T17:13:10Z

Just wondering why the anchor is created at each call but that's fine either way.

dmethvin · 2014-12-09T17:18:49Z

i noticed that as well but decided it was better to make sure it was "clean" rather than cache it somehow. document.createElement is pretty fast and this isn't time-critical code.

gibson042 · 2014-12-09T17:29:18Z

LGTM 👍

dmethvin · 2014-12-11T01:54:58Z

@mastahyeti I was about ready to land this but in running through our supported browsers I'm getting fails in every version of IE. Can you take a look?

btoews · 2014-12-11T05:04:57Z

What is an IE?

I just downloaded an IE 11 VM and it looks like they always add the port to a.host, but not for location.host. I'm pretty sure that this is the issue, but I'll double check when I get back to the computer I setup CI on.

btoews · 2014-12-11T17:08:12Z

The Ajax tests in master don't seem to pass in IE10:

I'm pulling an IE11 VM to try. Regardless though, the changes I just pushed do fix the tests that were failing from the location.host inconsistency.

mgol · 2014-12-11T17:31:47Z

@mastahyeti They did pass for us: http://swarm.jquery.org/job/3970. Might be some flakeyness; make sure you use an up-to-date IE10 version as well.

btoews · 2014-12-11T18:29:14Z

Hmm. I just ran Windows updates and I'm seeing the same test failure on master. Same thing in IE11.

dmethvin · 2014-12-11T18:39:44Z

Just updated and installed all Windows Updates. I don't see any test fails on master using IE InPrivate Browsing. I have some known incompatible plugins that mess up the results if I don't use InPrivate. Same could be happening for you? In my case I'm pretty sure it's LastPass.

I'll pull the updated PR in a few minutes and see if everything works here.

btoews · 2014-12-11T19:01:06Z

Sorry. I'm I didn't realize that it was using dis/jquery.js and that this wasn't getting rebuilt when I switched to master. This is a problem in my branch.

Ajax: don't use new url parsing for url rewriting Ajax: cut down on unnecessary variables Ajax: fix indentation of multi-line statement Ajax: parse origin URL using an anchor tag too (for IE) Ajax: some IE fixes. IE only does URL cleanup on for .href IE also throws errors for malformed URLs.

btoews · 2014-12-11T19:39:03Z

src/ajax.js

This accommodates some weird IE behavior.

Holy crap. Welcome to our cross-browser hell.

Let's add a support comment about that.

I'll get that.

Note sure if this will help, but here's a very similar solution with a giant comment about IE: https://github.com/angular/angular.js/blob/master/src/ng/urlUtils.js#L25

Yeah it seems to boil down to almost the same thing.

btoews · 2014-12-11T19:41:21Z

src/ajax.js

I wasn't sure if this should just fail closed, or cancel the request. If the URL is malformed, the request will end up failing either way...

Yeah, did some head-scratching on this one and left it as-is with an enhanced comment.

Fixes gh-1875 Closes gh-1880 (cherry picked from commit 5a75278e4c5359e07303fc4d8e78a1cf94f6ad65) Conflicts: src/ajax.js

dmethvin · 2014-12-11T22:15:01Z

Whew. This would have been so much easier if we had crummier unit tests and test matrix. 😈 I think both branches are good now, please take a look at the commits above and let me know if anything looks amiss. Thanks @mastahyeti!

btoews · 2014-12-11T22:18:49Z

Thanks for all the help @dmethvin

Fixes gh-1875 Closes gh-1880

All support comments were checked for Edge applicability.

dmethvin mentioned this pull request Dec 5, 2014

Preserve URL hash when requesting via $.ajax #1732

Closed

gibson042 reviewed Dec 6, 2014
View reviewed changes

src/ajax.js Outdated

Copy link

Member

gibson042 Dec 6, 2014

Choose a reason for hiding this comment

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

This should still be using s.url like the code it replaces.

gibson042 mentioned this pull request Dec 7, 2014

Misc: Drop support for older browsers; update support comments #1837

Merged

timmywil added the Ajax label Dec 10, 2014

dmethvin self-assigned this Dec 10, 2014

dmethvin added this to the 1.11.2/2.1.2 milestone Dec 11, 2014

btoews reviewed Dec 11, 2014
View reviewed changes

dmethvin pushed a commit that referenced this pull request Dec 11, 2014

Ajax: use anchor tag for parsing urls

de7ae8c

Fixes gh-1875 Closes gh-1880 (cherry picked from commit 5a75278e4c5359e07303fc4d8e78a1cf94f6ad65) Conflicts: src/ajax.js

dmethvin closed this in b091fdb Dec 11, 2014

mgol modified the milestones: 3.0.0, 1.11.2/2.1.2 Dec 15, 2014

staabm mentioned this pull request Jun 1, 2015

fix cross origin check for ie defunkt/jquery-pjax#531

Open

dmethvin mentioned this pull request Jul 15, 2015

Simplify $.ajax parsing using a link element #1875

Closed

markelog pushed a commit that referenced this pull request Nov 10, 2015

Ajax: use anchor tag for parsing urls

0986fe1

Fixes gh-1875 Closes gh-1880

dmethvin mentioned this pull request Nov 20, 2015

removed variable self-assignment #2735

Closed

mgol referenced this pull request Jun 8, 2016

Docs: Update support comments related to IE

693f1b5

All support comments were checked for Edge applicability.

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Ajax: use anchor tag for parsing urls #1880

Ajax: use anchor tag for parsing urls #1880

Uh oh!

Conversation

btoews commented Nov 23, 2014

Uh oh!

btoews commented Nov 24, 2014

Uh oh!

dmethvin commented Dec 1, 2014

Uh oh!

btoews commented Dec 1, 2014

Uh oh!

btoews commented Dec 2, 2014

Uh oh!

btoews commented Dec 2, 2014

Uh oh!

dmethvin commented Dec 2, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

btoews commented Dec 9, 2014

Uh oh!

jaubourg commented Dec 9, 2014

Uh oh!

jaubourg commented Dec 9, 2014

Uh oh!

dmethvin commented Dec 9, 2014

Uh oh!

gibson042 commented Dec 9, 2014

Uh oh!

dmethvin commented Dec 11, 2014

Uh oh!

btoews commented Dec 11, 2014

Uh oh!

btoews commented Dec 11, 2014

Uh oh!

mgol commented Dec 11, 2014

Uh oh!

btoews commented Dec 11, 2014

Uh oh!

dmethvin commented Dec 11, 2014

Uh oh!

btoews commented Dec 11, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmethvin commented Dec 11, 2014

Uh oh!

btoews commented Dec 11, 2014

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

9 participants