Uh oh!
There was an error while loading. Please reload this page.
Fix js initializer merging - #24050
Conversation
Still doesn't work correctly for multiple merges
Ryan Cavanaugh (RyanCavanaugh)
commented
May 11, 2018
Porting to 2.8 |
Ryan Cavanaugh (RyanCavanaugh)
commented
May 11, 2018
Bug is #24015 |
Ryan Cavanaugh (RyanCavanaugh)
commented
May 11, 2018
Since we don't need this fix in master ASAP, let's get some test cases / asserts added to this PR. I'm running manual VS tests in the 2.8 port branch |
Port fix#24050 from master to release-2.8
I added a test case and an assert that immediately catches the problem (1) in compilation (2) in fourslash (3) in interactive VS Code usage, so I think this is ready to go. |
Ryan Cavanaugh (RyanCavanaugh)
commented
May 11, 2018
Looks good. Can you add another fourslash test for the other repro we got? |
Ryan Cavanaugh (@RyanCavanaugh) can you send me the other minified repro? Since we didn't change the definition of getJSInitializerSymbol, and the binder code that creates symbols based on getJSInitializer is a single code path, I don't think it will prove anything new, but the cost of adding a second test is low. Note that without the fix the assert fires for existing tests too: to wit, the ones that had changed baselines. |
Ryan Cavanaugh (RyanCavanaugh)
commented
May 11, 2018
The two repros I have are and |
Fixes exponential merging behaviour of special js initialisers. Some baselines change because it fixes some previously incorrect merged symbol structure.
I have not tested it with Visual Studio, but it's basically the same as the fix we had working this afternoon, except that it doesn't break any of our existing tests.
Fixes#24015