Repository navigation
Add interface to cover UpdateRecorderBase's "apply" method #16003
Description
Activity
Can you provide an example use case?
commitUpdatewas intended to only be used in combination withbeginUpdate.So in the past month I've had a bit more time to think about this, so I'm going on a bit of a rant before and after the example. Sorry in advance! I love the tool! :)
So
beginUpdate(...)is a glorified constructor to give us anUpdateRecorderBase/Bomhidden behind theUpdateRecorderinterface.commitUpdate(…)then further has a runtime check to make sure the passed inUpdateRecorderis ONLY aninstanceofUpdateRecorderBaseotherwise it bombs out. I take it this is because thecommitUpdate(...)of theHostTreeisn't even using theUpdateRecorderinterface, so to get around the type checking it's justinstanceof'd. This also means there is a hard stop on extending the idea of a "transactional-diff-based-file-update" since you bring along the baggage ofinsertLeft(…)/insertRight(…)/remove(…)if you extendUpdateRecorderBase.Example outline:
Lets say I want to create a class that will provide a better interface for handling Typescript files. We'll call this theTypescriptFileUpdateRecorderclass. It can have it's own interface that instead of describing a very genericinsertLeft(…)andinsertRight(…)we provide a niceaddFileImport(…). In fact, we don't even want to provide the option toinsertLeft(…)orinsertRight(…)for user safety.Now we want to pass this
TypescriptFileUpdateRecorderback to theHostTree, but cannot, because it needs to implement an interface that is not even used by thecommitUpdate(…)function, and even if it did haveinsertLeft(…)andinsertRight(…)the runtimeinstanceofcheck would blow it all up.Example:
export class TypescriptFileUpdateRecorder implements UpdateRecorder { constructor(public entry: FileEntry) { } insertLeft(…) { // We don't want this. But need to implement it because of the interface's contract. } insertRight(…) { // We don't want this. But need to implement it because of the interface's contract. } remove(…) { // We don't want this. But need to implement it because of the interface's contract. } addFileImport(…) { // Do the background work of adding a file import to the typescript file. // Probably using the UpdateBuffer. That thing is cool. } apply(…){ // The function to take all the diffs we recorded // and apply them to the original file content. } } var component = tree.get('C:\Projects\app.component.ts') var tsRecorder = new TypescriptFileUpdateRecorder(component); tsRecorder.addFileImport('ControlValueAccessor', '@angular/forms'); tree.commitUpdate(tsRecorder);So in conclusion:
- Change
UpdateRecorderinterface to be what theHostTree.commitUpdate(…)is actually using. Namely:path: Pathandapply(content: Buffer). Maybe evenoriginal: Bufferdepending on howapply(…)is handled. - Remove the
instanceofcheck incommitUpdate(…)so user definedUpdateRecorder's can be used.
Some problems:
- This would be a breaking change. So those are never fun.
beginUpdate(...)kind of loses all purpose and can/should probably be retired.- There may be some confusion on who should implement the ContentHasMutatedException. There are arguments for either
HostTree.commitUpdate(…)handling it, and for theUpdateRecorderto handle it. Seems like theHostTreeshould handle that, sinceUpdateRecorderis exactly how it sounds. It records updates. Not confirm it's overwriting the correct target file.
So, thoughts?
- Change
Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.
Find more details about Angular's feature request process in our documentation.
Thank you for submitting your feature request! Looks like during the polling process it didn't collect a sufficient number of votes to move to the next stage.
We want to keep Angular rich and ergonomic and at the same time be mindful about its scope and learning journey. If you think your request could live outside Angular's scope, we'd encourage you to collaborate with the community on publishing it as an open source package.
You can find more details about the feature request process in our documentation.
Thank you for taking the time to submit this feature request.
At this time, we are not planning to implement this feature, as it did not gather sufficient community interest and we want to be mindful of expanding the CLI configuration surface area when existing workarounds are available.
We will be closing this issue as not planned for now, though we are happy to reconsider it in the future if community interest changes.
🚀 Feature request
Command (mark with an
x)Description
Currently when writing schematics, the
HostTreeexpects anUpdateRecorderBasefor it'scommitUpdate(...)function. This sharply limits creating custom schematic helpers, since any change needs to be done with an actualUpdateRecorderBase.Describe the solution you'd like
Include a new
UpdateRecordinterface (however you want to call it) that outlines theUpdateRecorderBase'sapply()function and remove the need for theUpdateRecorderBasespecifically to be used with theHostTree.commitUpdate(...)I have no problems cloning the repo and making the changes myself, but I would prefer some discussion around the subject to make sure it's in-line with the rest of the schematics implementations.
Describe alternatives you've considered
I looked into implementing my own
TreeInterface, but theHostTreeI would base it on uses a number of "backend" specific functionality that is not exported. Not to mention it would be incredibly tedious to re-implement this very basic class, when we are only trying to extend how an update occurs against theTree.Links
HostTree
UpdateRecorderBase
UpdateRecorder