Core: autoDocument to control setDocument() auto-switch - patching #292

[wojwal]

Patching #292

This change doesn't really solve #292 - but it's a quick-fix that at least give a way to disable setDocument() automatic document switch - when it's not needed or not wanted. One of the cases I was struggling with is that inside iframe - I had interval that was checking sth small from parent window, like $(element).offset() - and then inside interval it continues working with iframe elements. I also had mutation listener that is watching for new elements added to DOM. Sizzle was constantly adding temporary elements. And the performance compared to old jQuery (1.8.3 - that doesn't have setDocument) was devastated. Basically setDocument was happening twice with each setInterval callback.

Possible usage to disable setDocument auto document switch:

jQuery.find.autoDocument = false;

By default it's obviously enabled

[jquerybot]

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.

---

If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

[wojwal]

Hi,

Any update with this patch?

[markelog]

We would need tests for this.

But I'm not optimistic about this approach since first of all this a "external" (exposed to the user) fix of "internal" issue (ideally user shouldn't even know we doing this) and we would need to document/support this afterwards and probably expose it through jQuery API, which is not ideal. Plus we reaaaaly don't like global switches and made our policy to get rid of them... we consider them poor design nowadays.

Maybe you could come up with a bit more elegant solution for this?

[wojwal]

I understand - and I agree. That's why I'm calling it a patch and not a fix for #292

Unfortunately I don't have enough time right now to get deeper into Sizzle to make a real fix (although I'm hoping I will be able to get back to this in future). And the patch I suggested just turns on/off this document switch functionality - so I don't think it require much testing - it just allows to go back to the way sizzle worked old days.

I'm really hoping you (or someone from jquery team) will allow this patch - pls notice #292 is opened since October 2014 - and since then nobody coded a solution for it.

But yes - once real fix would be provided with a proper caching mechanism - above "autoDocument" global switch would be no longer needed.

Also - no need for any extra exposing to jQuery - sizzle is already exposed via "jQuery.find" - I was testing it with 2.2.4 version

[dmethvin]

In #292 I suggested a second copy of Sizzle to avoid using a single copy of Sizzle that has to switch documents. Does that address the problem for you? How many documents are you juggling at once? What kind of performance hit are you seeing?

[wojwal]

Well, one of the cases I was having that caused big performance downgrade was an an interval that once for 250 ms was checking position -- $(element).offset() -- of an element in parent frame - basically the iframe was supposed to be following position of an element - and adjust itself if it changes. And sizzle was reloading itself twice each 250ms...

When I was reviewing it I also had Mutation events watching new elements added to DOM - and I saw bunch of temporary elements added to DOM each time Sizzle switches document.

Another case is happening in Firefox extension. I load 1 instance of jQuery - and use it for XUL and different documents once user visit websites (basically when DOMContentLoaded occurs), like -- $("selector", document) -- It was also causing sizzle to reload each time. I would have to load new jQuery instance for each DOMContentLoaded in extension.

Anyway - from my side it was much simpler to google and eventually find #292, then patch jQuery/Sizzle...

I just thought others could also find this "autoDocument" config helpfull.

[markelog]

so I don't think it require much testing

Everything need to be tested or at least have a really good reason for it

pls notice #292 is opened since October 2014 - and since then nobody coded a solution for it.

That's probably because it is an edge case which not a lot of people face

Also - no need for any extra exposing to jQuery - sizzle is already exposed via "jQuery.find"

I meant exposed like this – https://github.com/jquery/jquery/blob/2d4f53416e5f74fa98e0c1d66b6f3c285a12f0ce/src/selector-sizzle.js

Accessing it through jQuery.find will work but it is not documented, more like a hack

[wojwal]

Hi guys, any update?

Everything need to be tested or at least have a really good reason for it

Could you pls test it? This patch is very small and easy :)

I meant exposed like this – https://github.com/jquery/jquery/blob/2d4f53416e5f74fa98e0c1d66b6f3c285a12f0ce/src/selector-sizzle.js

Accessing it through jQuery.find will work but it is not documented, more like a hack

Well, I must disagree. My intention was not to provide it for regular jQuery users. But rather for people - that are facing similar issue as me - and are forced to google for answer, and eventually find & realize that there is something called Sizzle that's in the back of jQuery ;)

I mean Sizzle already have some exposable configuration properties same as what I'm suggesting - for instance "cacheLength: 50" - it doesn't exposes directly to jQuery (and I think there is no need), but can be changed via jQuery.expr, like:

jQuery.expr.cacheLength = 123;

And as I already wrote - the way "autoDocument" can be accessed is:

jQuery.find.autoDocument = false;

Same type of hack (as you are saying) and I really think it should stay like this. Sizzle doesn't expose any configuration to jQuery and I don't think this "autoDocument" should be exception here.

Guys, pls accept this patch until real fix is introduced - on that time it can be removed, there is no point of exposing it to jQuery, it's really internal Sizzle problem. I guess #292 should remain opened - this gives at least alternative for people that will eventually find #292 in google like me. What dmethvin suggested (2'nd copy of jQuery) - is also not bad - but not in all cases, unfortunately not in mine.

[mgol]

Note: in jQuery 4.0 that no longer uses Sizzle (instead inlining it & simplifying greatly) setDocument will not do as much as it does right now in Sizzle. This is the current code on master, without comments it's just:

function setDocument( node ) {
	var subWindow,
		doc = node ? node.ownerDocument || node : preferredDoc;

	if ( doc === document || doc.nodeType !== 9 ) {
		return;
	}

	document = doc;
	documentElement = document.documentElement;
	documentIsHTML = !jQuery.isXMLDoc( document );

	if ( preferredDoc !== document &&
		( subWindow = document.defaultView ) && subWindow.top !== subWindow ) {

		subWindow.addEventListener( "unload", unloadHandler );
	}
}

(code from https://github.com/jquery/jquery/blob/29a9544a4fb743491a42f827a6cf8627b7b99e0f/src/selector.js#L419-L445)
and it may even get smaller before the final release, who knows.

[mgol]

It looks from the comments here the approach proposed in this PR was rejected. If anyone wants to tackle the issue, this comment from Richard Gibson would be a good start: #292 (comment).