Add interface to cover UpdateRecorderBase's "apply" method

[ShawnOzark]

🚀 Feature request

Command (mark with an x)

- [ ] new
- [ ] build
- [ ] serve
- [ ] test
- [ ] e2e
- [x] generate
- [ ] add
- [ ] update
- [ ] lint
- [ ] xi18n
- [ ] run
- [ ] config
- [ ] help
- [ ] version
- [ ] doc

Description

Currently when writing schematics, the HostTree expects an UpdateRecorderBase for it's commitUpdate(...) function. This sharply limits creating custom schematic helpers, since any change needs to be done with an actual UpdateRecorderBase.

Describe the solution you'd like

Include a new UpdateRecord interface (however you want to call it) that outlines the UpdateRecorderBase's apply() function and remove the need for the UpdateRecorderBase specifically to be used with the HostTree.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 the HostTree I 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 the Tree.

Links

HostTree
UpdateRecorderBase
UpdateRecorder

[clydin]

Can you provide an example use case? commitUpdate was intended to only be used in combination with beginUpdate.

[ShawnOzark]

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 an UpdateRecorderBase/Bom hidden behind the UpdateRecorder interface. commitUpdate(…) then further has a runtime check to make sure the passed in UpdateRecorder is ONLY an instanceof UpdateRecorderBase otherwise it bombs out. I take it this is because the commitUpdate(...) of the HostTree isn't even using the UpdateRecorder interface, so to get around the type checking it's just instanceof'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 of insertLeft(…)/insertRight(…)/remove(…) if you extend UpdateRecorderBase.

Example outline:
Lets say I want to create a class that will provide a better interface for handling Typescript files. We'll call this the TypescriptFileUpdateRecorder class. It can have it's own interface that instead of describing a very generic insertLeft(…) and insertRight(…) we provide a nice addFileImport(…). In fact, we don't even want to provide the option to insertLeft(…) or insertRight(…) for user safety.

Now we want to pass this TypescriptFileUpdateRecorder back to the HostTree, but cannot, because it needs to implement an interface that is not even used by the commitUpdate(…) function, and even if it did have insertLeft(…) and insertRight(…) the runtime instanceof check 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 UpdateRecorder interface to be what the HostTree.commitUpdate(…) is actually using. Namely: path: Path and apply(content: Buffer). Maybe even original: Buffer depending on how apply(…) is handled.
• Remove the instanceof check in commitUpdate(…) so user defined UpdateRecorder'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 the UpdateRecorder to handle it. Seems like the HostTree should handle that, since UpdateRecorder is exactly how it sounds. It records updates. Not confirm it's overwriting the correct target file.

So, thoughts?

[angular-robot]

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.

[angular-robot]

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.