Skip to content

Update src/manipulation.js#1113

Closed
Sysaninster wants to merge 1 commit intojquery:masterfrom
Sysaninster:patch-1
Closed

Update src/manipulation.js#1113
Sysaninster wants to merge 1 commit intojquery:masterfrom
Sysaninster:patch-1

Conversation

@Sysaninster
Copy link

It code bring leak of memory (test in chrome 23.0.1271.97 m, windows 7). Because every call $('selector').text('any text'), create 2 DOM nodes, but .empty() can't remove they. Code for test http://jsfiddle.net/3RSWf/ (I tested with source from repository and v1.8.3) and result http://my.jetscreenshot.com/7184/20130106-r5yo-219kb . It pull request fix problem.

It code bring leak of memory (test in chrome 23.0.1271.97 m, windows 7). Because every call $('selector').text('any text'), create 2 DOM nodes, but .empty() can't remove they. Code for test http://jsfiddle.net/3RSWf/ (I tested with source from repository and v1.8.3) and result http://my.jetscreenshot.com/7184/20130106-r5yo-219kb . It pull request fix problem.
@mikesherov
Copy link
Member

Thanks for contributing! In order for us to consider this pull request, you need to do several things:

  1. Download the repository, instead of editing directly in Github's editor.
  2. Follow our style guide ( your change is mis indented and uses spaces instead of tabs)
  3. Run the test suite to prove you didn't break anything ( in our supported browsers).
  4. Run grunt on the code, which does code quality checks like JSHint.
  5. Reference the open ticket in our bug tracker that you're attempting to fix.
  6. Write a unit test that both exposes the bug you're fixing and shows how your change fixes that bug.

Thanks again for contributing.. When you've done those things, please open a new pull request. For now I'm going to close this one.

@mikesherov mikesherov closed this Jan 6, 2013
@dmethvin
Copy link
Member

dmethvin commented Jan 6, 2013

Also, this particular patch was made against master. Since we want to keep 1.9 and 2.0 in sync behavior-wise, there will also need to be a 1.9 version of the patch for oldIE where there is no .textContent.

@mikesherov
Copy link
Member

Please see here for best practices on submitting a patch: https://github.com/jquery/jquery/blob/master/CONTRIBUTING.md#tips-for-jquery-bug-patching

@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
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.

3 participants