Fix bugs caused by html-safe empty strings #791
Conversation
|
Awesome, thank you for digging into this! |
Adding empty strings as HTML leads to difficulties keeping track of sibling nodes. It's easier to keep track of as text.
mongoose700
force-pushed
the
htmlsafe-empty-string
branch
from
5fa38ff to
bed226f
Compare
|
It seems to be failing some IE tests in CI, even though those tests don't even encounter my changes. Are they known to be flakey? |
|
I’ll restart, I’m not sure if they are normally flakey or not... |
mongoose700
force-pushed
the
htmlsafe-empty-string
branch
from
16a46ee to
bed226f
Compare
|
It failed an IE test again. The previous version that used a text node also failed on that test some times, but passed other times. It may be a flake. |
|
I kicked it once more... |
|
Latest build had a single failure in IE11: |
|
How do you normally go about debugging issues that only happen in IE? |
|
The "easiest" thing would be to download an IE11 virtual machine (from modern.ie) and run the tests... |
|
I've run the tests on a virtual machine repeatedly, and they never fail locally. Though I'm using Windows 8, and CI is using Windows 10. I'll see if I can get a different VM. |
|
I've tried with Windows 10, and the test still passes. I switched to QUnit 2.0.0 to match what CI is using, and updated IE, which took me past the version used in CI. Is there a way I can use the exact same VM that CI is using?
|
Likely related: emberjs/ember.js#14924 emberjs/ember.js#16172 emberjs/ember.js#16314 Closes #791 Co-authored-by: Michael Peirce <[email protected]>
|
@mongoose700 sorry this took so long! I merged in your changes in #852 and credited you for the commit. Would you mind opening a new PR to add the #each test you have here? |
Likely related: emberjs/ember.js#14924 emberjs/ember.js#16172 emberjs/ember.js#16314 Closes #791 Co-authored-by: Michael Peirce <[email protected]>
|
Nice, thanks for getting those changes in! I was having a hard time getting it to pass the tests for IE. |
Adding empty strings as HTML leads to difficulties keeping track of
sibling nodes. It's easier to keep track of as text.
This fixes the issues emberjs/ember.js#14924 and emberjs/ember.js#16314. The first one could be solved by simply removing the
if (html === null || html === '')special case. The latter issue cannot, because it would still need to deal with anulllast node.