fix(migrations): resolve text replacement issue#59452
fix(migrations): resolve text replacement issue#59452crisbeto wants to merge 1 commit intoangular:mainfrom
Conversation
Based on angular#59353 (comment), fixes a text replacement issue that seems to cause code to be duplicated in some setups.
| recorder | ||
| .remove(c.data.position, c.data.end - c.data.position) | ||
| .insertLeft(c.data.position, c.data.toInsert); | ||
| .insertRight(c.data.position, c.data.toInsert); |
There was a problem hiding this comment.
@devversion I think that historically we were using insertRight for migrations, but the tsurge-based ones seem to lean towards insertLeft instead. Maybe we should switch all of them over?
There was a problem hiding this comment.
Hmm. I don't see why it would fix duplications. do you understand?
(not opposed changing; but sounds like it's dependent on the order of replacements rather?)
There was a problem hiding this comment.
I'm just speculating since I haven't been able to reproduce it (see #59353 (comment)), but we've had other weird text replacement issues after the CLI swapped out their string buffer implementation, especially if ranges end up overlapping. My thinking is that when we do insertLeft, the new text technically overlaps the range that was just deleted which is throwing it off. With insertRight it gets inserted after the removed range so it's fine.
There was a problem hiding this comment.
Hmm. I've been trying to play with it, but can't really reproduce: https://magic-string-playground.vercel.app/?state=N4Igxg9gJgpiBcICWBbADhATgFwATAEEAaXAIRIGEBfXAM0whVwHIxmBuAHQDtUMd8xMpRr1GLGBx58seQiXK5qdBk2ZQp3HgHptublhQBDADa5sMAB7YQRENkxHuAZ1qGj2JBG4IQ2gFT%2BPLi4-rgAKgCeaDC4kRAArpjmji5umMae3rgAFjCYMMGhuACaibhgTrgJzrHYeXQQJiYQAO5I3ADmuABupgkwzvBFQdwhYQACaEaOTCjOuAC0EQ0AskadSGAAyg4d3R3O2E5ghWPFUzNGTFhIm9ymSyuxt-ePHWgJ2CPaPDzzADoCigID0YAAKAAMJAATAB2ACU-2cAKMaBi3CgABkYLRsFCSJxkOhZPhRKoWNwYK0CFI-iAkdxAcDQRCAKwAFhIAA4AIyMwFojHY3H4zmE4n8OTk8TMKmtUh0rQMkBUIA.
I was under impression the CLI uses magic-string at this point
There was a problem hiding this comment.
So we got another report about this bug in #59466 and it has a reproduction. It's really weird, because I can reproduce the bug on my end and I can verify that the fix works, but I can't reproduce it in our testing setup. I have a feeling that some test-specific logic might be normalizing it.
devversion
left a comment
There was a problem hiding this comment.
LGTM as I don't think this is a problem to merge, but I don't fully understand what would be best.
|
This PR was merged into the repository by commit 966a370. The changes were merged into the following branches: main, 19.1.x |
Based on #59353 (comment), fixes a text replacement issue that seems to cause code to be duplicated in some setups. PR Close #59452
Adds a test for the fix from angular#59452 now that we know what caused the issue.
Based on angular#59353 (comment), fixes a text replacement issue that seems to cause code to be duplicated in some setups. PR Close angular#59452
Adds a test for the fix from angular#59452 now that we know what caused the issue. PR Close angular#59497
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Based on #59353 (comment), fixes a text replacement issue that seems to cause code to be duplicated in some setups.