Skip to content
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

Bind this.onError for XHR requests in TurboGraft.Remote #119

Merged
merged 3 commits into from
Jun 23, 2016

Conversation

davidcornu
Copy link
Contributor

@davidcornu davidcornu self-assigned this Jun 22, 2016
@@ -33,7 +33,7 @@ class TurboGraft.Remote
triggerEventFor 'turbograft:remote:start', @initiator,
xhr: xhr

xhr.addEventListener 'error', @onError
xhr.addEventListener 'error', @onError.bind(@)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you do .bind(this)?

@qq99
Copy link
Contributor

qq99 commented Jun 22, 2016

Looks like it might address a lot of #118

cc @Shopify/tnt

@davidcornu
Copy link
Contributor Author

By the looks of things, the success and fail options of TurboGraft.Remote don't have any test coverage. I'll add some.

@@ -33,7 +33,7 @@ class TurboGraft.Remote
triggerEventFor 'turbograft:remote:start', @initiator,
xhr: xhr

xhr.addEventListener 'error', @onError
xhr.addEventListener 'error', @onError.bind(this)
Copy link
Member

Choose a reason for hiding this comment

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

More CoffeeScript-ey solution is to use => in the method definition (which binds the method in the constructor).

@davidcornu davidcornu force-pushed the fix-remote-onerror branch 2 times, most recently from e0e9fe6 to e6120ed Compare June 22, 2016 22:20
@@ -116,7 +116,7 @@ class TurboGraft.Remote
enabledInputs.push(input)
enabledInputs

onSuccess: (ev) ->
onSuccess: (ev) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured I'd switch this to a fat arrow too seen as it's always used as a callback

@davidcornu
Copy link
Contributor Author

@qq99 & @lemonmade - round 2 please 😁

httpRequestType: "POST"
httpUrl: "/foo/bar"
refreshOnError: "foo"
fail: done
Copy link
Member

Choose a reason for hiding this comment

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

If this never failed, would this test just timeout? Might be worth adding the success case that throws a meaningful error.

@qq99
Copy link
Contributor

qq99 commented Jun 23, 2016

Just going back to look at dafb3a7#diff-58f1b55d4025fab95951247202574f55L156 to analyze some more...

Looks like previously, @refreshOnError would have been undefined in some cases, and so it'd just do a general triggerEventFor and that'd be the end of it. It also looks like the network-level failures were/are the ones with problem (lacking proper this), combined with trying to do multi-level object lookup now

@davidcornu LGTM let me know when you'd like me to 🎩 this against core

@davidcornu
Copy link
Contributor Author

It also looks like the network-level failures were/are the ones with problem (lacking proper this), combined with trying to do multi-level object lookup now

Yeah that was my conclusion.

@davidcornu
Copy link
Contributor Author

@qq99 should be good to go. LMK if you want any help with 🎩 .

@qq99
Copy link
Contributor

qq99 commented Jun 23, 2016

Tested it in a few scenarios (order actions, some 422s) and looks good 🎩

@qq99
Copy link
Contributor

qq99 commented Jun 23, 2016

🚢

@davidcornu davidcornu merged commit cd29e42 into master Jun 23, 2016
@davidcornu davidcornu deleted the fix-remote-onerror branch June 23, 2016 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants