History native calls #3006

akre54 · 2014-02-19T14:03:36Z

Split from #3003, this removes Backbone.$ from Backbone.History and leaves View untouched.

dependency in History

akre54 · 2014-02-19T14:23:54Z

@jdalton can you verify this works in all versions of IE? My VM isn't booting on me.

braddunbar · 2014-02-19T14:48:00Z

backbone.js

+        removeEventListener('popstate', this.checkUrl);
+      } else if (this._wantsHashChange && 'onhashchange' in window && !this._oldIE) {
+        removeEventListener('hashchange', this.checkUrl);
+      }


Need to pass false for useCapture here as well, no?

* No need for `_oldIE` when we can just use `this.iframe`. * Added `this._hasHashchange`.

braddunbar · 2014-02-19T15:24:19Z

👍 This is good stuff. There's a quick tweak or two in akre54#2, tested in IE7-9.

jashkenas · 2014-02-19T15:25:58Z

@braddunbar — You like this change in general? Or only in conjunction with other patches that make it easier to untangle jQuery?

Remove `_oldIE` variable.

braddunbar · 2014-02-19T15:36:33Z

I like this one in general, regardless of what we decide about View. Removing the dependency from History is nice and has the bonus of reducing Backbone.$ API usage.

braddunbar · 2014-02-19T15:42:51Z

backbone.js

+          );
+          this.iframe = body.appendChild(frame).contentWindow;
+        }
+      } catch(e){}


Can we remove the try/catch if we check 'documentMode' in document instead?

The try-catch is also for the createElement call. Although that likely shouldnt matter if your check is there.

I also seem to recall that Safari doesnt expose document.body until the page has finished downloading (i.e. scripts in the head tag that use it are broken)

We were using document.body already so I'm not too worried about that. I'm not certain about documentMode though.

braddunbar · 2014-02-19T15:44:34Z

Removed isExplorer RegEx in akre54#3.

Remove `isExplorer` RegEx.

braddunbar · 2014-02-19T15:53:27Z

One more in akre54#4. 😃

jdalton · 2014-02-19T17:03:37Z

@jdalton can you verify this works in all versions of IE? My VM isn't booting on me.

I'll run it through the VMs again this evening. To see if anything changed after the cleanup.

acstll · 2014-02-19T20:11:23Z

Glad to see that after all the discussion on #2959 (trolling included) we might end up having native Router and a View you can cleanly patch regardless of Backbone.$

👍

Check `documentMode` and combine checks.

jdalton · 2014-02-20T05:58:25Z

@akre54 Tested latest from history-native-hooks in IE6-11, and IE9 in IE7 compat mode, all unit tests pass.

Remove `onpopstate` check.

Add a comment about clearInterval.

akre54 · 2014-02-21T15:28:49Z

backbone.js

@@ -1448,7 +1455,16 @@
    // Disable Backbone.history, perhaps temporarily. Not useful in a real app,
    // but possibly useful for unit testing Routers.
    stop: function() {
-      Backbone.$(window).off('popstate', this.checkUrl).off('hashchange', this.checkUrl);
+      // Add a cross-platform `removeEventListener` shim for older browsers.
+      var removeEventListener = window.removeEventListener || function (eventName, listener) {


@braddunbar Just wondering, but do we have to get rid of the iframe here? It looks like it just hangs around in the DOM with no listeners after stop is called, and creates a new one on each start. Potential memory leak?

Yep yep, we should remove it. Good catch!

We only store a reference to the iframe's contentWindow. Any idea how to turn this into a reference we can remove from the DOM?

Hmm...let's punt on it for this issue and address it separately. It's not a regression in any case.

braddunbar · 2014-02-21T16:51:45Z

documentMode is only present in IE>=8 so everything else hits the createElement. I updated with comments just before you asked above.

jashkenas · 2014-02-21T17:00:25Z

That seems quite a bit nastier than simply sniffing the UA. We shouldn't be attempting to create an iframe in environments where we know that none is needed in the first place.

jdalton · 2014-02-21T17:09:28Z

That seems quite a bit nastier than simply sniffing the UA.

Relying on a unique and relevant behavior to the target browsers is better than an arbitrary UA sniff. It also reduces the code needed to create the iframe, the try-catch is barely there.

We shouldn't be attempting to create an iframe in environments where we know that none is needed in the first place.

True, it could be done under a if (this._wantsHashChange) check.

jashkenas · 2014-02-21T17:28:49Z

Relying on a unique and relevant behavior to the target browsers is better than an arbitrary UA sniff.

In this particular case, hogwash. It's not at all out of the realm of plausibility that a new browser introduced in future years would allow HTML to be passed to document.createElement.

jdalton · 2014-02-21T17:34:45Z

In this particular case, hogwash.

It's not hogwash, and it simplifies creating the iframe element at the expense of ~15 chars for the try-catch.

It's not at all out of the realm of plausibility that a new browser introduced in future years would allow HTML to be passed to document.createElement.

Specs try to avoid breaking the web, doing research into api usage, and this area is well paved with IE opting to actually align with spec and drop the behavior in later releases. If your hangup is on the specific feature test that can be adjusted.

wyuenho · 2014-02-21T17:36:00Z

Can we feature detect oldIE with this?

var oldIE = document.compatMode || (document.documentMode || 0) < 8;

jdalton · 2014-02-21T17:37:09Z

Can we feature detect oldIE with this

Others have document.compatMode, at least Chrome.

wyuenho · 2014-02-21T17:37:46Z

Ergh

wyuenho · 2014-02-21T18:00:50Z

http://www.nczonline.net/blog/2009/12/29/feature-detection-is-not-browser-detection/

https://github.com/jquery/jquery-migrate/blob/master/src/core.js#L50

As much as I'd like to feature detect unique old IE features, I just don't think it's reliable. IE 6 was widespread enough that everyone else started copying over stuff. @jashkenas is right. The try catch createElement block is a little too clever and I think it will trip people up during maintenance.

wyuenho · 2014-02-21T18:17:39Z

I have a stupid question:

Why can't we do something like this?

if (!this._hasHashChange && this._wantsHashChange &&
   (!this._wantsPushState || !this._hasPushState)) {
  // make iframe
}

Why go thru the trouble detecting old browser features and/or UA sniff at all?

braddunbar · 2014-02-21T18:22:41Z

That's a great question. :)

jdalton · 2014-02-21T18:28:24Z

As much as I'd like to feature detect unique old IE features, I just don't think it's reliable.

Backbone is already employing feature inferences. If the createElement inference makes core queezy the check could be modeled after the existing noXhrPatch combo'ed with a docMode check of 0 || > 7 which would combined be an inference for more modern browsers to avoid creating the iframe.

braddunbar · 2014-02-21T23:58:02Z

I like @wyuenho's approach in akre54#10. If neither hashChange or pushState are supported, History will be broken anyway so there's no obvious downside.

jdalton · 2014-02-22T00:00:31Z

I like @wyuenho's approach in akre54#10. If neither hashChange or pushState are supported, History will be broken anyway so there's no obvious downside.

Can dig.

…port

Create iframe conditional on options and hashchange and pushstate support

braddunbar · 2014-02-27T14:04:17Z

Looking good, just ran the tests in IE7-9 successfully. 👍

braddunbar · 2014-02-27T14:04:38Z

...and also a test page to ensure routing worked correctly.

jashkenas · 2014-02-27T15:59:17Z

So this looks ready to go ... but it seems like a patch that only really makes sense in conjunction with one of the native-view patches. By itself, it simply uses jQuery a little bit less.

Should the current state of this changed be merged into the open native-view patches ... instead of being merged directly?

wyuenho · 2014-02-27T16:07:56Z

I think this can be merged as step 1, this PR and #3003 don't have a direct dependency like in #2959. This PR is also blocking #3015. The sooner this gets merge the sooner #3015 can be fixed.

jashkenas · 2014-02-27T16:14:14Z

I think this can be merged as step 1, this PR and #3003 don't have a direct dependency like in #2959. This PR is also blocking #3015. The sooner this gets merge the sooner #3015 can be fixed.

Not so fast. #3015 is not a real problem. Fixing it is fine, but it doesn't matter.

I understand that this PR doesn't have a direct dependency — but that's not the question. This change, looked at in isolation, is a step backwards. If we merge it, and then never merge the two-steps-forward, then that's no good.

Instead, let's try adding it in to the proposed patches that would rely on it (or make use of it).

akre54 · 2014-02-27T16:20:55Z

To be fair #3015 isnt a showstopper -- by a long shot. Should be no harm in merging this now and working out the kinks in View in the next few days

akre54 · 2014-02-27T16:22:48Z

This change, looked at in isolation, is a step backwards.

How so? One could avoid jquery entirely if they only wanted routing and not views. We're almost there with #3003.

braddunbar · 2014-02-27T17:00:31Z

This change, looked at in isolation, is a step backwards.

I disagree. Removing the jQuery dependency allows use of Backbone.History without extra steps in cases that don't use Backbone.View. Otherwise, it requires jQuery or a shim. I'd call that a step forward on it's own.

Further, it removes the browser sniffing and fixes a long standing issue regarding starting history before the page loads. All wins in my book.

jashkenas · 2014-02-27T18:05:31Z

Alright then.

History native calls

akre54 added 5 commits February 19, 2014 08:58

add in addEventListener shim from @wyuenho (0844b00) to remove jQuery

6ed0cdd

dependency in History

add window.popstate checks for IE7 backwards compat

1115997

cleanup oldIE sniff code and iframe creation per @jdalton

0101416

remove first window argument from add/removeEventListener

5d305bf

check for onpopstate in window instead of window.onpopstate

4ab05ad

braddunbar reviewed Feb 19, 2014
View reviewed changes

Remove _oldIE variable.

573070f

* No need for `_oldIE` when we can just use `this.iframe`. * Added `this._hasHashchange`.

braddunbar and others added 2 commits February 19, 2014 10:28

Pass useCapture to removeEventListener.

87dce15

Merge pull request #2 from braddunbar/history

501d7bf

Remove `_oldIE` variable.

braddunbar reviewed Feb 19, 2014
View reviewed changes

Remove isExplorer RegEx.

18e15e2

Merge pull request #3 from braddunbar/history

e5eeea4

Remove `isExplorer` RegEx.

Check documentMode and combine checks.

2c50dc5

akre54 added 2 commits February 19, 2014 23:56

improve inline comments

91ef885

Merge pull request #4 from braddunbar/history

feeab3f

Check `documentMode` and combine checks.

braddunbar and others added 2 commits February 20, 2014 06:17

Remove onpopstate check.

b1bcf0a

Merge pull request #6 from braddunbar/history

a91f6ac

Remove `onpopstate` check.

jashkenas added the change label Feb 20, 2014

Merge pull request jashkenas#3011 from braddunbar/clear-interval

a717a71

Add a comment about clearInterval.

wyuenho mentioned this pull request Feb 21, 2014

Native view and jQuery-less History #2959

Closed

akre54 reviewed Feb 21, 2014
View reviewed changes

wyuenho mentioned this pull request Feb 21, 2014

Create iframe conditional on options and hashchange and pushstate support akre54/backbone#10

Merged

wyuenho and others added 2 commits February 22, 2014 22:48

Create iframe conditional on options and hashchange and pushstate sup…

674f101

…port

Merge pull request #10 from wyuenho/history-native-hooks

16b6f1e

Create iframe conditional on options and hashchange and pushstate support

jashkenas added a commit that referenced this pull request Feb 27, 2014

Merge pull request #3006 from akre54/history-native-hooks

1853297

History native calls

jashkenas merged commit 1853297 into jashkenas:master Feb 27, 2014

akre54 mentioned this pull request Mar 5, 2014

Add optional dependencies / exoskeleton support: first step marionettejs/backbone.marionette#768

Closed

i-love-code mentioned this pull request Jun 17, 2014

Major performance boost. Fix #126 cloudflarearchive/backgrid#444

Open

History native calls #3006

History native calls #3006

Conversation

akre54 commented Feb 19, 2014

akre54 commented Feb 19, 2014

Choose a reason for hiding this comment

braddunbar commented Feb 19, 2014

jashkenas commented Feb 19, 2014

braddunbar commented Feb 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

braddunbar commented Feb 19, 2014

braddunbar commented Feb 19, 2014

jdalton commented Feb 19, 2014

acstll commented Feb 19, 2014

jdalton commented Feb 20, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

braddunbar commented Feb 21, 2014

jashkenas commented Feb 21, 2014

jdalton commented Feb 21, 2014

jashkenas commented Feb 21, 2014

jdalton commented Feb 21, 2014

wyuenho commented Feb 21, 2014

jdalton commented Feb 21, 2014

wyuenho commented Feb 21, 2014

wyuenho commented Feb 21, 2014

wyuenho commented Feb 21, 2014

braddunbar commented Feb 21, 2014

jdalton commented Feb 21, 2014

braddunbar commented Feb 21, 2014

jdalton commented Feb 22, 2014

braddunbar commented Feb 27, 2014

braddunbar commented Feb 27, 2014

jashkenas commented Feb 27, 2014

wyuenho commented Feb 27, 2014

jashkenas commented Feb 27, 2014

akre54 commented Feb 27, 2014

akre54 commented Feb 27, 2014

braddunbar commented Feb 27, 2014

jashkenas commented Feb 27, 2014