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

smaller/stronger domManip/buildFragment/clean #680

Closed
wants to merge 3 commits into from

Conversation

@gibson042
Copy link
Member

commented Feb 10, 2012

jQuery Size - compared to last make
  249375   (-809) jquery.js
   93929   (-289) jquery.min.js
   33345   (-109) jquery.min.js.gz
@rwaldron

This comment has been minimized.

Copy link
Member

commented Feb 10, 2012

This does way more then address the bug in the ticket... I'm sorry, but after I re-opened the ticket, I wrote a test and tiny patch for this that only addresses the bug in question.

@rwaldron

This comment has been minimized.

Copy link
Member

commented Feb 10, 2012

Actually, I would recommend pulling my 11323 branch and laying your changes on top - for the sake of having the tests at least.

@rwaldron

This comment has been minimized.

Copy link
Member

commented Feb 10, 2012

Or better - file a ticket for the "other stuff". It's a pain to keep track of monster changes like this that don't actually fit within the scope of the ticket your trying to resolve.

@rwaldron

This comment has been minimized.

Copy link
Member

commented Feb 10, 2012

More notes: Manipulation is failing an append test in IE 6 & 7

@gibson042

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2012

Done. I apologize, and hope that creating a new ticket and renaming this PR suffices.

@gibson042

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2012

jQuery Size - compared to last make
  249537   (-649) jquery.js
   94000   (-218) jquery.min.js
   33348   (-106) jquery.min.js.gz
@dmethvin

This comment has been minimized.

Copy link
Member

commented Feb 14, 2012

There's a lot of great nip/tuck in this but I'd like to wait until 1.8 to land it due to the possibility of regressions. Objections?

Now if I could just figure out why I can't tag tickets...

@gibson042

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2012

I strongly support the idea of holding this for 1.8.

@jaubourg

This comment has been minimized.

Copy link
Member

commented Feb 14, 2012

+1 on waiting for 1.8

@gibson042

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2012

Is there anything I need to do here for conflict resolution (rebasing perhaps?) now that PR 689 has landed, or will everything be taken care of with that merge?

@rwaldron

This comment has been minimized.

Copy link
Member

commented Mar 3, 2012

One thing that should be noted, despite jQuery.fn.domManip and jQuery.buildFragment not being documented, we've seen time and time again that devs use APIs without regard for documentation (eg. event data). Changing formal parameter list order is a change that...

  1. Breaks backwards compatibility
  2. Gives jQuery.fn.domManip an awkward API where the callback is no longer the last argument (this occurs elsewhere, thanks to unfortunate legacy)

I just tried resolving the conflicts this creates by favoring these changes and there seems to be a significant divergence that can't be resolved, so I'll leave it to you to sort that out, since you know your changes better then I

@rwaldron

This comment has been minimized.

Copy link
Member

commented Mar 3, 2012

A few more thoughts... this change set disfavors a lot of simple conditions, that are accompanied by comments and ticket numbers in favor of "clever" ternary and lazy evaluation assignment expressions. jQuery used to be full of these and they were hard to maintain and often unclear in their intention, which is why a lot of new code began to use complete _IfStatement_s, with thorough explanations.

Example: https://github.com/jquery/jquery/pull/680/files#L1L475 specifically, 475-480 => 465-466

@mikesherov

This comment has been minimized.

Copy link
Member

commented Mar 3, 2012

@gibson042, you should definitely do your own rebase here, as it's going to be tough for anyone else to resolve the conflicts aside from you.

@gibson042

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2012

I resolved everything by backing out the conflicting code changes from 619f0d9 and rebuilding this branch on top of the result.

New PR: #704

@gibson042 gibson042 closed this Mar 7, 2012
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.