Skip to content
This repository has been archived by the owner on Sep 25, 2021. It is now read-only.

Move call to .bind behind a Turbolinks.supported check #284

Merged
merged 1 commit into from May 4, 2017

Conversation

trueheart78
Copy link
Contributor

@trueheart78 trueheart78 commented May 4, 2017

Internet Explorer 8 continued to raise an exception when attempting to use Turbolinks v5.0.2. We kept getting, TypeError: Object doesn't support this property or method for .bind(), which is used only in the ScrollManager constructor. In this same browser, Turbolinks.supported returns false, so this was not expected.

This PR changes it to check that Turbolinks is supported prior to calling .bind().

Worked with @JakeTobin on this to verify the cause and that this change provides a fix.

@nateberkopec
Copy link
Contributor

Maybe this should short-circuit earlier, though? TL obviously isn't expected to work with IE8, so maybe it should shut off sooner.

@packagethief
Copy link
Member

I agree, @nateberkopec. ScrollManager.start() is already behind a Turbolinks.supported check in the controller. Can we wait until start() to create the throttled scroll handler so it needn't be done in the constructor?

@trueheart78
Copy link
Contributor Author

trueheart78 commented May 4, 2017

@nateberkopec The one other spot it could work is in the Controller class, where the ScrollManager is instantiated. There is a support check in the start method, but not in the constructor.

@packagethief Would that cause an issue with calling start/stop, since the instantiation would be in the start method? To clarify, though, are we talking about putting it in the start() method in the ScrollManager, or in the Controller?

@packagethief
Copy link
Member

@trueheart78, yes, I was suggesting we put it in ScrollManager's start(). Here's one way to do it:

diff --git a/src/turbolinks/scroll_manager.coffee b/src/turbolinks/scroll_manager.coffee
index 1de743e..5368ee6 100644
--- a/src/turbolinks/scroll_manager.coffee
+++ b/src/turbolinks/scroll_manager.coffee
@@ -1,16 +1,16 @@
 class Turbolinks.ScrollManager
   constructor: (@delegate) ->
-    @onScroll = Turbolinks.throttle(@onScroll).bind(this)
 
   start: ->
     unless @started
-      addEventListener("scroll", @onScroll, false)
+      @throttledOnScroll ?= Turbolinks.throttle(@onScroll).bind(this)
+      addEventListener("scroll", @throttledOnScroll, false)
       @onScroll()
       @started = true
 
   stop: ->
     if @started
-      removeEventListener("scroll", @onScroll, false)
+      removeEventListener("scroll", @throttledOnScroll, false)
       @started = false

I bet @javan can think of a better way 😄

@javan
Copy link
Contributor

javan commented May 4, 2017

How about

diff --git a/src/turbolinks/scroll_manager.coffee b/src/turbolinks/scroll_manager.coffee
index 1de743e..98e38d1 100644
--- a/src/turbolinks/scroll_manager.coffee
+++ b/src/turbolinks/scroll_manager.coffee
@@ -1,6 +1,6 @@
 class Turbolinks.ScrollManager
   constructor: (@delegate) ->
-    @onScroll = Turbolinks.throttle(@onScroll).bind(this)
+    @onScroll = Turbolinks.throttle(@onScroll)
 
   start: ->
     unless @started
@@ -19,7 +19,7 @@ class Turbolinks.ScrollManager
   scrollToPosition: ({x, y}) ->
     window.scrollTo(x, y)
 
-  onScroll: (event) ->
+  onScroll: (event) =>

@trueheart78
Copy link
Contributor Author

Those both look like they would work. What should my next step be? Seeing commit hashes tells me you've both done some work.

@packagethief
Copy link
Member

@trueheart78, I'd update this PR using @javan's suggestion (and confirm it works). Thank-you!

@trueheart78
Copy link
Contributor Author

trueheart78 commented May 4, 2017

I updated it with @javan's suggestion, and force pushed. All tests still pass.

Will be testing it in IE8 shortly.

@trueheart78
Copy link
Contributor Author

IE8 is still working as-expected with the updated build.

@packagethief packagethief merged commit 185b405 into turbolinks:master May 4, 2017
@packagethief
Copy link
Member

Thanks @trueheart78!

@trueheart78
Copy link
Contributor Author

excited

Glad to help out, @packagethief @javan

@packagethief
Copy link
Member

Hey @trueheart78, not a big deal, but for future reference you don't need to commit the compiled JavaScript in dist/turbolinks.js when submitting a PR. We'll do that when we cut a release. My bad for not noticing! 😄

@trueheart78
Copy link
Contributor Author

Thanks for the info, @packagethief.

And thanks for the release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants