Simplify $.ajax parsing using a link element #1875

Closed
dmethvin opened this Issue Nov 20, 2014 · 12 comments

Comments

Projects
None yet
3 participants
@dmethvin
Member

dmethvin commented Nov 20, 2014

Currently we use a relatively complex regex to parse the URL and determine same domain requests for choosing the correct transport. We culd use the trick of parsing the url via an <a> element. Should be less hassle and less code.

@dmethvin dmethvin added the Ajax label Nov 20, 2014

@dmethvin dmethvin added this to the 3.0.0 milestone Nov 20, 2014

dmethvin added a commit that referenced this issue Dec 11, 2014

Ajax: use anchor tag for parsing urls
Fixes gh-1875
Closes gh-1880
(cherry picked from commit 5a75278e4c5359e07303fc4d8e78a1cf94f6ad65)

Conflicts:
	src/ajax.js

@dmethvin dmethvin closed this in b091fdb Dec 11, 2014

@phistuck

This comment has been minimized.

Show comment
Hide comment
@phistuck

phistuck Jul 15, 2015

Will you accept a patch that uses new URL instead (where available)?

Will you accept a patch that uses new URL instead (where available)?

@jaubourg

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Jul 15, 2015

Member

So nice to see this disapear! \o/

Member

jaubourg commented Jul 15, 2015

So nice to see this disapear! \o/

@phistuck

This comment has been minimized.

Show comment
Hide comment
@phistuck

phistuck Jul 15, 2015

@jaubourg - were you responding to my question? :) If so, is that a yes?

@jaubourg - were you responding to my question? :) If so, is that a yes?

@jaubourg

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Jul 15, 2015

Member

@phistuck, I was just sharing my happiness regarding the disappearance of the regexp. Using the URL API is a good idea though but we'd probably need to wait for all supported browsers to implement it (I haven't checked if they all do right now). The general consensus is to avoid multiple code paths, especially for small, non-critical, parts of the code like this one.

Member

jaubourg commented Jul 15, 2015

@phistuck, I was just sharing my happiness regarding the disappearance of the regexp. Using the URL API is a good idea though but we'd probably need to wait for all supported browsers to implement it (I haven't checked if they all do right now). The general consensus is to avoid multiple code paths, especially for small, non-critical, parts of the code like this one.

@phistuck

This comment has been minimized.

Show comment
Hide comment
@phistuck

phistuck Jul 15, 2015

@jaubourg - AJAX is used very frequently and the cost of creating an element in terms of memory (and possible garbage collection issues in the engine) is pretty big, compared to the URL constructor. Looks like everyone but Internet Explorer supports it.
Since the interface should be identical to the one <a> exposes (there share the interface), it should be a small change ($.isFunction(window.URL) ? new URL() : document.createElement("a")).

@jaubourg - AJAX is used very frequently and the cost of creating an element in terms of memory (and possible garbage collection issues in the engine) is pretty big, compared to the URL constructor. Looks like everyone but Internet Explorer supports it.
Since the interface should be identical to the one <a> exposes (there share the interface), it should be a small change ($.isFunction(window.URL) ? new URL() : document.createElement("a")).

@jaubourg

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Jul 15, 2015

Member

Well, this piece code is only executed once, not at each request. I agree it's a small change but I fail to see how critical it is. It seems rather logical to just wait for our support grid to only contain browsers that implement the URL object before simply switching to it.

I don't speak for the team though but that's my take on it.

Member

jaubourg commented Jul 15, 2015

Well, this piece code is only executed once, not at each request. I agree it's a small change but I fail to see how critical it is. It seems rather logical to just wait for our support grid to only contain browsers that implement the URL object before simply switching to it.

I don't speak for the team though but that's my take on it.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jul 15, 2015

Member

During review of #1880 we discussed this but my take at the time was that it wasn't critical. Creating a link element a few times a second shouldn't be a perf or memory problem should it?

Member

dmethvin commented Jul 15, 2015

During review of #1880 we discussed this but my take at the time was that it wasn't critical. Creating a link element a few times a second shouldn't be a perf or memory problem should it?

@jaubourg

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Jul 15, 2015

Member

My bad, I hadn't scrolled enough to see it used in the ajax code proper :/

Still, I agree with Dave, perf is not an issue here.

Member

jaubourg commented Jul 15, 2015

My bad, I hadn't scrolled enough to see it used in the ajax code proper :/

Still, I agree with Dave, perf is not an issue here.

@phistuck

This comment has been minimized.

Show comment
Hide comment
@phistuck

phistuck Jul 15, 2015

Performance (in terms of speed) is less of an issue, but memory usage and failing to free the memory (the old Internet Explorer bug, for example) are concerning. Creating an element is a pretty big hammer and if you can avoid it with exactly the same interface, I do not see why not.

(But I understand your reasoning as well)

Performance (in terms of speed) is less of an issue, but memory usage and failing to free the memory (the old Internet Explorer bug, for example) are concerning. Creating an element is a pretty big hammer and if you can avoid it with exactly the same interface, I do not see why not.

(But I understand your reasoning as well)

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jul 15, 2015

Member

Are you saying there's a memory leak that new URL() would fix here?

Member

dmethvin commented Jul 15, 2015

Are you saying there's a memory leak that new URL() would fix here?

@phistuck

This comment has been minimized.

Show comment
Hide comment
@phistuck

phistuck Jul 15, 2015

I am not saying that there is one now (I have not checked it). I just say that an element is a big hammer that is better used sparingly when you have other options and I mentioned that browsers did use to have problems with element references and leaks. new URL is much more lightweight (and is the preferred way) than constructing <a>.
Developers used to create a whole new document for resolving URL components, for example, which is an even bigger hammer that new URL also replaces.

I am not saying that there is one now (I have not checked it). I just say that an element is a big hammer that is better used sparingly when you have other options and I mentioned that browsers did use to have problems with element references and leaks. new URL is much more lightweight (and is the preferred way) than constructing <a>.
Developers used to create a whole new document for resolving URL components, for example, which is an even bigger hammer that new URL also replaces.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jul 15, 2015

Member

If new URL() worked everywhere there would be a better case for changing this, but as it is I'm not seeing any concrete problem here that requires a change.

Since the interface should be identical to the one exposes (there share the interface), it should be a small change ($.isFunction(window.URL) ? new URL() : document.createElement("a")).

It should but may not be. If there was a provable issue here I think we'd be more likely to change the code.

Member

dmethvin commented Jul 15, 2015

If new URL() worked everywhere there would be a better case for changing this, but as it is I'm not seeing any concrete problem here that requires a change.

Since the interface should be identical to the one exposes (there share the interface), it should be a small change ($.isFunction(window.URL) ? new URL() : document.createElement("a")).

It should but may not be. If there was a provable issue here I think we'd be more likely to change the code.

markelog added a commit that referenced this issue Nov 10, 2015

@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016

@cssmagic cssmagic referenced this issue in cssmagic/ChangeLog May 18, 2016

Open

jQuery #5

@jquery jquery locked as resolved and limited conversation to collaborators Jun 19, 2018

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