feat(tree): Include list of properties that changed in nodeChanged event #22043
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 135659d The changes in this PR will be included in the next version bump. This PR includes changesets to release 157 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
area: dds
|
✅ Successfully linked to Azure Boards work item(s): |
github-actions
bot
added
the
area: framework
⯅ @fluid-example/bundle-size-tests: +1.56 KB
Baseline commit: 06f2989 |
|
Do we plan to generalize this to apply to treeChanged in the future? I could see the same functionality being great there as well. That also makes me thing: I don't think this includes a test where we change a node deeper in the tree (ex: a child) and make sure the parent doesn't get a nodeChanged, of when it does, keys inly deeply edited don't get included in the changed key list (like they would be in treeChanged). Anyway, the parts I looked at (API, changeset and tests) generally look good to me, though I left some suggestions. |
noencke
reviewed
Jul 31, 2024
| @@ -390,7 +390,9 @@ export interface TreeArrayNodeUnsafe<TAllowedTypes extends Unenforced<ImplicitAl | |||
|
|
|||
| // @public @sealed | |||
| export interface TreeChangeEvents { | |||
| nodeChanged(): void; | |||
| nodeChanged({ changedProperties }: { | |||
| changedProperties: ReadonlySet<string>; | |||
There was a problem hiding this comment.
I'm a little concerned about forward compat (for when we finally expose information about what the changes were).
Is there a design for what that will look like?
Are we expecting to just deprecate changedProperties and use another key for the future API? (propertyChanges ?)
Or would we be better off exposing changedProperties: ReadonlyMap<string, unknown> for now, so we can fill in the blanks later?
There was a problem hiding this comment.
I think the fact that changedProperties is inside of a property bag {...} (as opposed to directly being the first parameter to the event) is sufficient for future-proofing and about as good of a guess as we can make. To add more information in the future, we don't need to break the event signature, we just add more properties to the event bag. Maybe there's some overlap with the existing properties and a new property supersedes an old one, but that's fine, the user can just ignore the deprecated one.
There was a problem hiding this comment.
I'm cool with that so long as we're going into it with our eyes open.
Feel free to resolve this comment without further changes.
tylerbutler
dismissed
their stale review
Dismissing since there's a changeset now. Haven't reviewed it yet but don't block on me.
Josmithr
reviewed
Jul 31, 2024
| */ | ||
| export function getViewForForkedBranch<TSchema extends ImplicitFieldSchema>( | ||
| originalView: SchematizingSimpleTreeView<TSchema>, | ||
| ): { forkView: SchematizingSimpleTreeView<TSchema>; forkCheckout: ITreeCheckoutFork } { |
There was a problem hiding this comment.
Should we define a type for this? That would give us a better place to document it properties and such.
Josmithr
reviewed
Jul 31, 2024
| nodeChanged(): void; | ||
| nodeChanged({ | ||
| changedProperties, | ||
| }: { readonly changedProperties: ReadonlySet<string> }): void; |
There was a problem hiding this comment.
Should we define a type for this?
This was referenced Aug 15, 2024
|
I created #22229 which merges main into this and fixed up a few things. |
CraigMacomber
added a commit
that referenced
this pull request
Aug 16, 2024
…g. (#22222) ## Description See changeset for details. This will be used for things like providing object node specific events with strong typing. This is aimed to provide the functionality attempted in alexvy86#7 to better support node kind specific data in #22043
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This adds a payload to the
nodeChangedevent in simple-tree, where we specify the names of the properties of the node that changed and caused the event to fire.TODO (in this PR)
Reviewer Guidance
The review process is outlined on this wiki page.
AB#8399