UpdateRecorder should support multiple adjacent inserts

[dgp1130]

🐞 Bug report

Command (mark with an x)

• schematics

Is this a regression?

No

Description

From #20190.

UpdateRecorder doesn't support multiple adjacent inserts.

🔬 Minimal Reproduction

it('works with multiple adjacent inserts', () => {
  const buffer = Buffer.from('Hello beautiful World');
  const entry = new SimpleFileEntry(normalize('/some/path'), buffer);

  const recorder = new UpdateRecorderBase(entry);
  recorder.remove(6, 9);
  recorder.insertRight(6, 'amazing');
  recorder.insertRight(15, ' and fantastic');
  const result = recorder.apply(buffer);
  expect(result.toString()).toBe('Hello amazing and fantastic World');
});

Anything else relevant?

Best workaround is probably to use something more comprehensive like magic-string, but that probably shouldn't be required for something this simple.

[kyubisation]

Due to my limited understanding of the context of the UpdateRecorder I don't know if my solution suggestion is valid.

One way to do this, would be to track all changes in a list without mutating the original. When calling apply(), all changes are applied sequentially, sorted descending by index.

A naive implementation:
https://stackblitz.com/edit/node-4trosz?devtoolsheight=33&file=index.ts (See below)
Run npm test to run the tests (which use a mock jasmine implementation)

[kyubisation]

Updated the StackBlitz example to use jasmine:
https://stackblitz.com/edit/node-7gdaqv?devtoolsheight=33&file=index.ts
Run npm test to run the tests

[kyubisation]

So, I've been looking at the UpdateBuffer spec and I have to admit I don't understand the requirements. Is this complexity really required?

[dgp1130]

@kyubisation, I don't think this complexity is strictly required. We do recommend using magic-string as a more complete implementation. We are hoping to refactor in the future to drop this custom implementation in favor of the real magic-string. Should we just close this issue and wait for that?

[kyubisation]

That's fine with me.
Would you accept an external contribution or is this something that should be done "internally"?

[dgp1130]

I think we would probably accept a PR as it is something which should work and is a legitimate bug. The existence and recommendation of magic-string just means that this is lower priority and we probably won't go out of our way to fix this particular issue. That said, definitely open to improvements here!

I think I'll leave the issue open just because it is a real issue that should be fixed eventually (whether with a direct fix or a switch to magic-string).

[kyubisation]

I have implemented a PR, switching usage of UpdateBuffer to magic-string: #21209
As mentioned in the PR, this is just an initial implementation and I'm open to suggestions for improvement.

[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.}