[1.9] Fix #11795 and #10470: keep scripts in DOM; execute only on first insertion #864

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
3 participants
@gibson042
Member

gibson042 commented Jul 17, 2012

http://bugs.jquery.com/ticket/11795 didn't really generate the discussion for which I was hoping, but maybe a pull request is just the ticket, so to speak. These changes revert the funkiness of our <script> handling (http://jsfiddle.net/jR3H6/1/) and instead explicitly track their execution with a private datum so we can evaluate only on first insertion. See new tests at the end of test/unit/manipulation.js to get a sense of the changes.

Suggestions, comments, and improvements are all welcome.

Sizes - compared to master @ bea5ecb

    268733      (+794)  dist/jquery.js                                         
     93905      (+275)  dist/jquery.min.js                                     
     33607      (+145)  dist/jquery.min.js.gz
@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Oct 31, 2012

Member

I think this one is finally ready. It's big, but probably manageable... and I'd like to sweep over src/manipulation.js after http://bugs.jquery.com/ticket/11795 is resolved anyway.

Member

gibson042 commented Oct 31, 2012

I think this one is finally ready. It's big, but probably manageable... and I'd like to sweep over src/manipulation.js after http://bugs.jquery.com/ticket/11795 is resolved anyway.

- if ( scripts.length ) {
- jQuery.each( scripts, function( i, elem ) {
- if ( elem.src ) {
- if ( jQuery.ajax ) {

This comment has been minimized.

@mikesherov

mikesherov Oct 31, 2012

Member

where'd this go?

@mikesherov

mikesherov Oct 31, 2012

Member

where'd this go?

+
+ if ( node.src ) {
+ // Hope ajax is available...
+ jQuery.ajax({

This comment has been minimized.

@mikesherov

mikesherov Oct 31, 2012

Member

What happened to the modularity check for jQuery.ajax?

@mikesherov

mikesherov Oct 31, 2012

Member

What happened to the modularity check for jQuery.ajax?

This comment has been minimized.

@gibson042

gibson042 Oct 31, 2012

Member

I think it was removed in the pre-rebased branch for smaller size. I can add it back, but is Error: no ajax really better than TypeError: jQuery.ajax is not a function?

@gibson042

gibson042 Oct 31, 2012

Member

I think it was removed in the pre-rebased branch for smaller size. I can add it back, but is Error: no ajax really better than TypeError: jQuery.ajax is not a function?

This comment has been minimized.

@mikesherov

mikesherov Oct 31, 2012

Member

No, it's not. :-) But you should then make sure this test works with a custom build that has ajax excluded.

@mikesherov

mikesherov Oct 31, 2012

Member

No, it's not. :-) But you should then make sure this test works with a custom build that has ajax excluded.

This comment has been minimized.

@gibson042

gibson042 Nov 1, 2012

Member

You're a wily one, @mikesherov. Now that it's possible to test no-ajax builds, I can confirm that this PR passes. 😤

@gibson042

gibson042 Nov 1, 2012

Member

You're a wily one, @mikesherov. Now that it's possible to test no-ajax builds, I can confirm that this PR passes. 😤

This comment has been minimized.

@dmethvin

dmethvin Nov 2, 2012

Member

Thinking ahead a bit, calling jQuery.ajax directly is probably not a good idea here. If we let people drop in alternate ajax implementations they may not take the same args. What if we add a simple API to ajax.js that does this, and people can shim in their own if they replace jQuery.ajax?

@dmethvin

dmethvin Nov 2, 2012

Member

Thinking ahead a bit, calling jQuery.ajax directly is probably not a good idea here. If we let people drop in alternate ajax implementations they may not take the same args. What if we add a simple API to ajax.js that does this, and people can shim in their own if they replace jQuery.ajax?

This comment has been minimized.

@gibson042

gibson042 Nov 2, 2012

Member

That is an excellent point, but probably out of scope for this batch of changes. I'll open a new ticket.

@gibson042

gibson042 Nov 2, 2012

Member

That is an excellent point, but probably out of scope for this batch of changes. I'll open a new ticket.

This comment has been minimized.

@dmethvin

dmethvin Nov 2, 2012

Member

👍

This comment has been minimized.

@mikesherov

mikesherov Nov 2, 2012

Member

btw, this pull request is awe-inspiring.

@mikesherov

mikesherov Nov 2, 2012

Member

btw, this pull request is awe-inspiring.

test/unit/manipulation.js
@@ -1489,11 +1492,13 @@ test("html() on empty set", function() {
});
var testHtml = function(valueObj) {
- expect(35);
+ var expected = 36;
+ expect(expected);

This comment has been minimized.

@mikesherov

mikesherov Oct 31, 2012

Member

expected( 36 )

@mikesherov

mikesherov Oct 31, 2012

Member

expected( 36 )

This comment has been minimized.

@gibson042

gibson042 Oct 31, 2012

Member

I started with that. Then things got out of hand. 😉

@gibson042

gibson042 Oct 31, 2012

Member

I started with that. Then things got out of hand. 😉

@gibson042 gibson042 closed this in e889134 Nov 19, 2012

mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014

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