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

Bug 8322: Add plain object signature to DOM manipulation methods #239

Closed
wants to merge 2 commits into from

Conversation

timmywil
Copy link
Member

This will allow calls to append/prepend such as

$('div').append('<div/>', { id: "foo", class: "bar" });

I've added tests, but I'm not sure those are the best places. However, I feel I had to add new ones. I don't think I can integrate my tests into the common ones because bareObj and functionReturningObj only take one argument. Any review is appreciated.

Performance tests: http://jsperf.com/dh-dommanip-with-8322-patch/2

http://jsperf.com/dommanip/2

timmywil added 2 commits February 19, 2011 14:26
…d the plain object signature to dom manipulation methods originally requested in the forums
@leeoniya
Copy link

can you update the perf test to reflect 18408f4? thx!

@timmywil
Copy link
Member Author

It already is. I added the test when I added that commit. Compare with first revision at http://jsperf.com/dh-dommanip-with-8322-patch . The updated perf tests need more tests tho.

@leeoniya
Copy link

nvm, my bad. the performance gap is 50%-60% on first run and 10%-20% on reruns (diminishing with each rerun) :( jQuery.isPlainObject must be pretty slow. would be be nice to just rely on args.length, hehe.

@timmywil
Copy link
Member Author

It's strange to me that the first revision tests are that much slower even though it never hits the isPlainObject check. However, it sped up when I checked args[1] rather than checking length first. I created more tests to be sure.

@timmywil
Copy link
Member Author

This was not accepted. Closing.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants