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

Ajax: Create correct URLs for jQuery.ajax() with cache: false #3902

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@wenz

wenz commented Dec 22, 2017

fixes gh-3682

Summary

Correctly creates the URL for calls to jQuery.ajax() when the "cache" option is set to false. Fixes gh-3682.

Checklist

@mgol mgol added the Needs review label Dec 22, 2017

@gibson042

This comment has been minimized.

Member

gibson042 commented Dec 23, 2017

Is ?&_=… actually a problem? I know it's unusual, but it's also perfectly valid and properly interpreted by every server framework of which I'm aware.

We might accept a small fix regardless, but this PR increases the file size a lot:

   raw     gz Sizes
272031  80593 dist/jquery.js
 87420  30244 dist/jquery.min.js

   raw     gz Compared to master @ ecd8ddea33dc40ae2a57e4340be03faf2ba2f99b
  +336   +110 dist/jquery.js
   +91    +51 dist/jquery.min.js

You might be able to get it back down with a URL manipulation function, or with wicked clever regexes (one matching the part of a URL preceding a _ query parameter¹ and the other matching the longest "complete" prefix from a URL² for identifying a query append strategy)—but the savings would have to be dramatic to justify such increased complexity. However, I'd be willing to consider anything that passes all the tests from my example.

¹ /^([^?]*(?=\?)(.(.*(?=&)|)|).)_=[^&]*&?/
² /^([^?]*(?=\?)(.(.*(?=&).|)$|))/

@wenz

This comment has been minimized.

wenz commented Dec 23, 2017

Thanks for the feedback! The ?&_ URL is perfectly valid, but "does not look good". I'll have another pass at it with more Regex magic. We might also be able to save a few bytes if more or all regexes are defined within the IIFE (currently, some are defined outside the function and passed as an argument, others are defined inside).

@gibson042

This comment has been minimized.

Member

gibson042 commented Dec 23, 2017

If all we're talking about is aesthetics, I'd rather not add any size or complexity, but will definitely take a look. Another possible approach for "clean" parameter appending might be something like

(url + "?&").replace(/^([^?]*\?)(?:(.+?)&*\?(&)|\??&)$/, "$1$2$3") + "name=value"

@gibson042 gibson042 closed this Dec 23, 2017

@gibson042 gibson042 reopened this Dec 23, 2017

@wenz

This comment has been minimized.

wenz commented Jan 5, 2018

So I've messed around with your approach and also a few other things. Before I start cleaning up for a PR, just that we are on the same page, I was wondering about the correct test results for some of your tests in the fiddle.

  • What is the correct result for the five "tricky path" tests? My take: since the cash buster parameter is appended to the query string, tricky-path&_=OLD for instance needs to result in tricky-path&_OLD?_=NEW (since there was no question mark in the test URL)
  • What is the correct result for the "tricky query" test? I'd assume the second question mark ought to be escaped (%3F) for the URL to be correct, but what happens if we get this URL? My take: the first question mark starts the query string, and we have name-value pairs there, with = separating names from values. Therefore, the parameter name would be "tricky-query?_", and the value would be "OLD". The result should then be "tricky-query?_=OLD&_=NEW". In that case, your approach would pass all tests. :-) Alternatively, we could argue that the last question mark starts the query string (the regex can be changed easily to accomodate that), but that does not seem to be right.
@gibson042

This comment has been minimized.

Member

gibson042 commented Jan 5, 2018

What is the correct result for the five "tricky path" tests? My take: since the cash buster parameter is appended to the query string, tricky-path&_=OLD for instance needs to result in tricky-path&_OLD?_=NEW (since there was no question mark in the test URL)

Right. The table in the fiddle is all correct, and in fact optimal with the exception that the output from tricky-path&_=OLD?& could be tricky-path&_=OLD?_=NEW instead of tricky-path&_=OLD?&_=NEW.

What is the correct result for the "tricky query" test? I'd assume the second question mark ought to be escaped (%3F) for the URL to be correct, but what happens if we get this URL?

It is valid to escape question marks in the query component, but not necessary.

My take: the first question mark starts the query string, and we have name-value pairs there, with = separating names from values. Therefore, the parameter name would be "tricky-query?_", and the value would be "OLD". The result should then be "tricky-query?_=OLD&_=NEW".

Correct.

In that case, your approach would pass all tests. :-)

Yes, sorry I didn't make that clear.

Alternatively, we could argue that the last question mark starts the query string (the regex can be changed easily to accomodate that), but that does not seem to be right.

A non-percent-encoded question mark is not allowed in the path or any URI earlier component, so by definition the first such character starts the query component.

@vikram06

Approved

@timmywil timmywil removed the Needs review label Mar 5, 2018

@timmywil

This comment has been minimized.

Member

timmywil commented Mar 5, 2018

Still needs updates per @gibson042's fiddle and requests.

@timmywil timmywil added this to the 3.4.0 milestone Sep 3, 2018

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