Replace removeChild with remove#213465
Conversation
remcohaszing
force-pushed
the
remove-child-remove
branch
2 times, most recently
from
911d359 to
5cf9f14
Compare
This replaces most uses of `parent.removeChild(child)` with `child.remove()`. The two are almost equivalent. The only difference is that `parent.removeChild(child)` throws if the given node is not a child of the parent, whereas `child.remove()` never throws. There is no noticable performance difference. The only reason to use `removeChild` is to support Internet Explorer, but that’s no longer supported by Monaco editor.
The script content changed, so the sha256 hash changed too.
remcohaszing
force-pushed
the
remove-child-remove
branch
from
6d51dfb to
277b6e2
Compare
Co-authored-by: Logan Ramos <lramos15@gmail.com>
lramos15
left a comment
lramos15
left a comment
There was a problem hiding this comment.
Thanks for the information and the contribution
|
|
||
| <meta http-equiv="Content-Security-Policy" | ||
| content="default-src 'none'; script-src 'sha256-bQPwjO6bLiyf6v9eDVtAI67LrfonA1w49aFkRXBy4/g=' 'self'; frame-src 'self'; style-src 'unsafe-inline';"> | ||
| content="default-src 'none'; script-src 'sha256-noHVLQsurkONXmA3fcuAmcZ8UPYm/db88mhm9gAXcvk=' 'self'; frame-src 'self'; style-src 'unsafe-inline';"> |
There was a problem hiding this comment.
Looks good, it matches what I computed for the <script> tag at https://emn178.github.io/online-tools/sha256.html
|
@remcohaszing @lramos15 isnt this a bad change? Notice 4 remove calls after, and 2 before? 
Did we review this change throughly? |
|
Yes, you just beat me to this comment. It’s a sloppy mistake. It happens twice, right before and after that Do you want to go through with the full revert or should I just fix those occurrences? |
This fixes a regression caused by microsoft#213465 Closes microsoft#214303 Closes microsoft#214345
| if (defaultStyles.length) { | ||
| mainWindow.document.head.removeChild(defaultStyles[0]); | ||
| } | ||
| defaultStyles?.[0].remove(); |
There was a problem hiding this comment.
@remcohaszing this is not a good change, what if the array is empty?
There was a problem hiding this comment.
This was indeed wrong. Fixed it in #214348.
| catch (error) { | ||
| // Ignore, removed already by change of focus | ||
| } | ||
| this.selectDropDownContainer.remove(); // remove to take out the CSS rules we add |
There was a problem hiding this comment.
Could this throw? why was it try/catch before?
There was a problem hiding this comment.
No. parent.removeChild(child) throws if child is not a child of parent. child.remove() never throws. Aside from not needing a reference to the parent, that’s the only difference.
ghost
mentioned this pull request
6 participants
This replaces most uses of
parent.removeChild(child)withchild.remove().The two are almost equivalent. The only difference is that
parent.removeChild(child)throws if the given node is not a child of the parent, whereaschild.remove()never throws. There is no noticable performance difference. The only reason to useremoveChildis to support Internet Explorer, but that’s no longer supported by Monaco editor.I tested this in the Monaco playground, but not very thoroughly.