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 callback in rails ujs #29127

Merged
merged 1 commit into from Oct 19, 2017

Conversation

Projects
None yet
5 participants
@DmytroVasin
Contributor

DmytroVasin commented May 17, 2017

Steps to reproduce

Hi!
I'm using rails 5.1.1 and faced with next unexpected behaviour.

According to documentation Ajax request should not happen if I will return false from ajax:before or ajax:beforeSend events:

$('#form').on('ajax:beforeSend', function(event) {
  return false;
});

Even test suits in current Rails version check that. But tests stop at runtime - they just halt.

Tests: call-remote-callback module inside /actionview/test/ujs/test folder.

Expected behavior:

return false inside ajax:beforeSend should prevent request.

Actual behavior:

return false inside ajax:beforeSend does not prevent request.

Furthermore if BeforeSend Return false - send request happens

System configuration

Rails version:
5.1.1
Ruby version:
ruby 2.4.1p111 (2017-03-22 revision 58053) [x86_64-darwin15]

Other Information

In current PR:

If i did something wrong, missed something or remove useful stuff - please let me know.

Thanks!

@rafaelfranca rafaelfranca requested review from guilleiguaran and javan May 17, 2017

@DmytroVasin

This comment has been minimized.

Show comment
Hide comment
@DmytroVasin

DmytroVasin Jun 9, 2017

Contributor

Any news on that?

Anyway I simplified PR. Now it contains only fix of callback.

Contributor

DmytroVasin commented Jun 9, 2017

Any news on that?

Anyway I simplified PR. Now it contains only fix of callback.

@guilleiguaran guilleiguaran merged commit b7bf709 into rails:master Oct 19, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@guilleiguaran

This comment has been minimized.

Show comment
Hide comment
@guilleiguaran

guilleiguaran Oct 19, 2017

Member

@DmytroVasin thanks!! I was running UJS tests in a browser and noticed this as an observable bug in the test suite, merging your PR fixed it right away!!

Member

guilleiguaran commented Oct 19, 2017

@DmytroVasin thanks!! I was running UJS tests in a browser and noticed this as an observable bug in the test suite, merging your PR fixed it right away!!

@ta1kt0me

This comment has been minimized.

Show comment
Hide comment
@ta1kt0me

ta1kt0me Oct 26, 2017

Contributor

@guilleiguaran @DmytroVasin Hi, thanks for the fix! I'm looking forward releasing this. Is there any plan to backport this to 5.1.x ?

Contributor

ta1kt0me commented Oct 26, 2017

@guilleiguaran @DmytroVasin Hi, thanks for the fix! I'm looking forward releasing this. Is there any plan to backport this to 5.1.x ?

y-yagi added a commit to y-yagi/rails that referenced this pull request Dec 17, 2017

y-yagi added a commit that referenced this pull request Dec 17, 2017

@obnijnil

This comment has been minimized.

Show comment
Hide comment
@obnijnil

obnijnil Apr 14, 2018

I found return false in both ajax:before and ajax:beforeSend handlers like:

document.querySelector('form').addEventListener('ajax:beforeSend', function(event) {
  return false;
});

doesn't prevent the form request, but event.preventDefault(); does:

document.querySelector('form').addEventListener('ajax:beforeSend', function(event) {
  event.preventDefault();
});

Is it a bug, or am I misuse something?

Rails version: 5.1.4/5.2.0.

obnijnil commented Apr 14, 2018

I found return false in both ajax:before and ajax:beforeSend handlers like:

document.querySelector('form').addEventListener('ajax:beforeSend', function(event) {
  return false;
});

doesn't prevent the form request, but event.preventDefault(); does:

document.querySelector('form').addEventListener('ajax:beforeSend', function(event) {
  event.preventDefault();
});

Is it a bug, or am I misuse something?

Rails version: 5.1.4/5.2.0.

@DmytroVasin

This comment has been minimized.

Show comment
Hide comment
@DmytroVasin

DmytroVasin Apr 14, 2018

Contributor

@obnijnil Thanks for your comment,
In that PR I tried to provide my thoughts. Please look into it.

Thank you.

Contributor

DmytroVasin commented Apr 14, 2018

@obnijnil Thanks for your comment,
In that PR I tried to provide my thoughts. Please look into it.

Thank you.

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