feat(@angular-devkit/schematics): add UpdateBuffer2 based on magic-string

[kyubisation]

This PR adds UpdateBuffer2 which should eventually replace UpdateBuffer. UpdateBuffer2 internally uses the magic-string library.
UpdateBuffer and related symbols have been marked as deprecated.

Closes #21110

[kyubisation]

This is an initial implementation of the magic-string changes discussed in #21110. I'm open to suggestions for improvement.
Also while this is not a breaking change to the API, it does change internal behavior (see changed unit tests). Would this be considered a breaking change?

[clydin]

Thank you for the contribution.
I'll do a more thorough review soon but I wanted to address the breaking change question. Looking at the changes to the tests, this would indeed be breaking. As-is, this would need to go into a major version (v13 being the next in Fall 2021). To get this in sooner, both modes would need to be supported and controlled via an option. This option could potentially be exposed via beginUpdate but if there were a more appropriate place that would work as well. This would allow the current behavior to remain the default for 12.x but still allow the new behavior for those who need/prefer it. The default could also potentially be swapped in v13+ and then followed by an eventual removal.

[kyubisation]

This fix is not urgent for me, so landing it in v13 would be fine by me. I can change the deprecation notices accordingly.

As for providing it side-by-side; A possibility would be to pass a parameter through NodeWorkflowOptions => BaseWorkflowOptions => HostTree => beginUpdate.
I'm blanking a bit on the potential names. useMagicString for parameter and UpdateRecoderMagicString for the UpdateRecorder implementation?

[clydin]

For the option name, preference would be to treat magic-string as an implementation detail and keep it encapsulated if possible. Maybe a name such as compatibilityMode or similar (open to other ideas) with a default to true which would use the existing methodology.

[kyubisation]

An alternative for the configuration may be an environment variable (e.g. NG_LEGACY_UPDATE_RECORDER, NG_SAFE_UPDATE_RECORDER or simply NG_UPDATE_RECORDER_V2), as there are already precedents like NG_PERSISTENT_BUILD_CACHE.
This would avoid the need for the propagation.

[kyubisation]

@clydin I have updated the implementation. I have reverted previous changes to the existing UpdateBuffer and created an additional UpdateBuffer2, which now both extend UpdateBufferBase. I have implemented a switch based on the environment variable NG_UPDATE_BUFFER_V2.
I'm still very much open to suggestions for improvement.

[kyubisation]

@clydin I don't know whether the change to the goldens is acceptable in this case. I can revert the changes to HostSink, if it is a problem.

[kyubisation]

@clydin With 13 looming, could I ask for another review of this PR?

[clydin]

Sorry for the review delay.
This looks good for v13.

Can you rebase? There appears to be a conflict with package.json.

[alan-agius4]

Nit: deprecations comments should be updated to @deprecated Since v13.0

[kyubisation]

Thanks for the catch. Fixed accordingly.

[kyubisation]

@clydin No problem (in regards to the delay). I have rebased the branch.
One question; Should I switch to usage to default to UpdateBuffer2 (e.g. rename NG_UPDATE_BUFFER_V2 to NG_LEGACY_UPDATE_BUFFER and adapt accordingly) or would you like to continue using UpdateBuffer for v13?

[clydin]

Let's leave the default with the existing behavior for now. We may switch it for v13 but we would like to do some analysis regarding the magnitude of breakage before changing it.

Thank you again for the contribution.

[kyubisation]

Thank you for the review and other feedback. 😃

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