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
Fix AJAX data source error #4356
Conversation
Fixes select2#4355 Verified in Firefox & Chromium
I believe this the solution to a bug that is affecting all installation of select2. As such, shouldn't it be released in a patch version (4.0.4), rather than a minor version (4.1.0) ? It seems that including this for 4.1 is only delaying the fix without any other benefits. I don't think it's either a breaking change or a new feature. |
@PowerKiKi the next planned release of Select2 is 4.1.0, we have no intentions of rolling a 4.0.4 release due to (outside) time constraints. |
Not quite what I wanted to hear, but thanks for taking the time to answer anyway 👍 |
Currently experiencing bug #4355. Would love to see this PR merged or the next release rolled out so I can upgrade. Probably going to apply this patch anyway for the time being. But +1 this PR! |
👍 for a new minor version increase. This is a bugfix affecting most/all modern browsers! |
It seems that select2 lacks a proper workflow for (semi-)continuous delivery. It's a pity because bugs like these impact a lot of users, yet are very simple and safe to fix. Especially when the community do half the work to fix it. |
Any comment on when this will be released? Time frames? Experiencing the 4355 across all devices... |
We'll need regression tests before we can merge this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll also need a unit test before we can test this. /A priori/ it would make sense to add a test with request.status being 0
and another one for '0'
as a string, and ensure that it works as expected in both cases.
dist/js/select2.full.js
Outdated
@@ -3444,7 +3444,7 @@ S2.define('select2/data/ajax',[ | |||
}, function () { | |||
// Attempt to detect if a request was aborted | |||
// Only works if the transport exposes a status property | |||
if ($request.status && $request.status === '0') { | |||
if ('status' in $request && $request.status === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not going to break support for all browsers that have '0' ? shouldnt this be $request.status === 0 || $request.status === '0'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For those who are using timeout : if ('status' in $request && $request.status === 0 && $request.statusText != "timeout")
dist/js/select2.js
Outdated
@@ -3444,7 +3444,7 @@ S2.define('select2/data/ajax',[ | |||
}, function () { | |||
// Attempt to detect if a request was aborted | |||
// Only works if the transport exposes a status property | |||
if ($request.status && $request.status === '0') { | |||
if ('status' in $request && $request.status === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't change dist files, for more info check CONTRIBUTING
src/js/select2/data/ajax.js
Outdated
@@ -82,7 +82,7 @@ define([ | |||
}, function () { | |||
// Attempt to detect if a request was aborted | |||
// Only works if the transport exposes a status property | |||
if ($request.status && $request.status === '0') { | |||
if ('status' in $request && $request.status === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question.
@jpic done & submitted changes. I'm not familiar with JS testing, but will look into if I find some free time. If somebody else could take on the tests I'd be grateful. |
Seems like a pretty popular fix - can someone who knows their way around the testing framework please write some tests so we can get this released in the next revision? |
They updated to check for either integer or string values.
Just reminding everyone that this exists and it's been over a year. 😞 |
@lordplagus02 it will be released in 4.0.6. Hold yer horses 🐴 Actually, I've already merged it into |
Merged into |
Any idea when this is going to be released? |
@markoheijnen In next weeks. I am reviewing some another PRs. After it, we will release a version. |
Hey. Are you sure this solution indeed fixes the issue. At my company we are still having issues with it. Changing function at 3589 of select.js to below fixed the problem.
|
This pull request includes a
The following changes were made
Fixes #4355
Verified in Firefox & Chromium