refactor: update schematics to work with new UpdateBuffer

[alan-agius4]

This change is needed for the upcoming changes in Angular CLI version 16 where the default UpdateBuffer is replaced with the UpdateBuffer2.

This commits changes how certain updates are applied to avoid overlapping changes which would otherwise result in broken output due to the offset would be out of sync.

[devversion]

Is there a little more context on why this previously worked? The old update recorder always worked even if updates are applied not necessarily in reverse order. Just asking for better understanding.

[alan-agius4]

It works because of a bug which was leveraged in this case. We now switched to magic-string to better support multiple adjacent operations which previously were buggy.

For context here is the bug that was levraged

const buffer = Buffer.from('Hello beautiful World');
//Old 
const recorder = new UpdateBuffer(buffer);
recorder.remove(0, 5);
recorder.insertRight(0, Buffer.from('Hola amazing'));
recorder.remove(0, 5);
recorder.insertRight(0, Buffer.from(' and fantastic'));
recorder.toString() // Hola amazing and fantastic beautiful World

// New
const recorder2 = new UpdateBuffer2(buffer);
recorder2.remove(0, 5);
recorder2.insertRight(0, Buffer.from('Hola amazing'));
recorder2.remove(0, 5);
recorder2.insertRight(0, Buffer.from(' and fantastic'));
recorder2.toString() //  and fantastic beautiful World

[alan-agius4]

Note: this reverse "trick" is not something new as it's used in other places

• https://github.com/angular/angular/blob/dedac8d3f73ebf4f05b773454e2a22ab5fa4bf7c/packages/core/schematics/ng-generate/standalone-migration/util.ts#L124-L142

• components/src/material/schematics/migration-utilities/update.ts

  Lines 18 to 23 in c5b59bd

  	/** Applies the updates to the given file content in reverse offset order. */
  	export function writeUpdates(content: string, updates: Update[]): string {
  	updates.sort((a, b) => b.offset - a.offset);
  	updates.forEach(update => (content = update.updateFn(content)));
  	return content;
  	}

[devversion]

Yeah, we did it a couple of times, but also quite a few migrations (especially older ones) did not follow that pattern because the update recorder was assumed to intentionally handle this out of the box, only ever considering the original source indices.

I will need to experiment a bit to get a better understanding

[alan-agius4]

There is definitely something weird with this migration though as there was an attempt to correct the node positions before as well which is odd.

https://github.com/angular/components/pull/26504/files#diff-d1860aa442910133e59b78c36e7947c6a0921723c3cbd77a78223a4aa6ab8e1bR432

[devversion]

Yeah, those are not ideal. I'm good with this change anyway. I like the fact that magic-string is used under the hood. I just want to make sure we document the semantic changes well when Angular CLI releases. Arguably, the behavior you outlined in #26504 (comment) seemed expected to me and also reasonable to me. i.e. remove always operates on the original string indices. This makes a lot of sense.

Going forward the way remove works is definitely a breaking change, and personally I find it confusing given that the documentation of magic-string claims to only operate on the original string.

[alan-agius4]

Yeah that is indeed a confusing behaviour especially considering that docs state otherwise.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}