Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(tree): adds a changed event on TreeBranchEvents #22977

Merged
merged 19 commits into from
Nov 5, 2024

Conversation

jenn-le
Copy link
Contributor

@jenn-le jenn-le commented Nov 5, 2024

adds a new changed event to TreeBranchEvents that fires for both local and remote changes

@jenn-le jenn-le requested review from a team as code owners November 5, 2024 01:21
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: tree area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct changeset-present public api change Changes to a public API base: main PRs targeted against main branch labels Nov 5, 2024
packages/dds/tree/src/shared-tree/schematizingTreeView.ts Outdated Show resolved Hide resolved
packages/dds/tree/src/simple-tree/api/tree.ts Outdated Show resolved Hide resolved
packages/dds/tree/src/simple-tree/api/tree.ts Outdated Show resolved Hide resolved
@@ -631,6 +643,9 @@ export interface TreeBranchEvents {
* @param data - information about the commit that was applied
* @param getRevertible - a function provided that allows users to get a revertible for the commit that was applied. If not provided,
* this commit is not revertible.
*
* @deprecated use the "changed" event instead which behaves the same way but also gets fired for more scenarios, see its doc comment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably link here with {@link TreeBranchEvents.changed | "changed"}

packages/dds/tree/src/core/rebase/types.ts Outdated Show resolved Hide resolved
"section": tree
---

Added a new `changed` event to the (currently alpha) `TreeBranchEvents` that is meant to replace the `commitApplied` event on `TreeViewEvents`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Added a new `changed` event to the (currently alpha) `TreeBranchEvents` that is meant to replace the `commitApplied` event on `TreeViewEvents`.
Adds a new `changed` event to the (currently alpha) `TreeBranchEvents` that replaces the `commitApplied` event on `TreeViewEvents`.

---
"section": tree
---

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a concise description of the change? Examples can be found in #22965

Copy link
Contributor

@jzaffiro jzaffiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs look good!

@@ -580,6 +580,7 @@ export class TreeViewConfiguration<const TSchema extends ImplicitFieldSchema = I

// @public @sealed
export interface TreeViewEvents {
// @deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hold off on deprecating this for now. I'm not sure if we should deprecate something in the public API when the only alternative is an alpha API. Some users might not want to participate in alpha APIs, and we would be forcing them to choose between two evils.

If we change our mind, we can easily deprecate later, no harm done, and we will at the very latest deprecate when the other event becomes public.

@jenn-le jenn-le enabled auto-merge (squash) November 5, 2024 02:19
Copy link
Collaborator

@msfluid-bot msfluid-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Coverage Summary

↓ packages.dds.tree.src.core.rebase:
Line Coverage Change: No change    Branch Coverage Change: -0.03%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 96.99% 96.96% ↓ -0.03%
Line Coverage 99.46% 99.46% → No change
↑ packages.dds.tree.src.simple-tree.api:
Line Coverage Change: 0.04%    Branch Coverage Change: 0.03%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 87.85% 87.88% ↑ 0.03%
Line Coverage 82.19% 82.23% ↑ 0.04%
↑ packages.dds.tree.src.shared-tree:
Line Coverage Change: No change    Branch Coverage Change: 0.08%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 90.77% 90.85% ↑ 0.08%
Line Coverage 97.23% 97.23% → No change

Baseline commit: d7658b5
Baseline build: 304262
Happy Coding!!

Code coverage comparison check passed!!

@msfluid-bot
Copy link
Collaborator

@fluid-example/bundle-size-tests: +425 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 464.21 KB 464.24 KB +35 Bytes
azureClient.js 562.45 KB 562.5 KB +49 Bytes
connectionState.js 724 Bytes 724 Bytes No change
containerRuntime.js 261.86 KB 261.87 KB +14 Bytes
fluidFramework.js 424.73 KB 424.84 KB +104 Bytes
loader.js 134.17 KB 134.19 KB +14 Bytes
map.js 42.71 KB 42.71 KB +7 Bytes
matrix.js 148.54 KB 148.55 KB +7 Bytes
odspClient.js 528.3 KB 528.34 KB +49 Bytes
odspDriver.js 97.84 KB 97.86 KB +21 Bytes
odspPrefetchSnapshot.js 42.81 KB 42.83 KB +14 Bytes
sharedString.js 164.76 KB 164.76 KB +7 Bytes
sharedTree.js 415.19 KB 415.29 KB +97 Bytes
Total Size 3.36 MB 3.36 MB +425 Bytes

Baseline commit: a475dc4

Generated by 🚫 dangerJS against 9eca00e

@jenn-le jenn-le merged commit e51c94d into microsoft:main Nov 5, 2024
32 checks passed
Josmithr pushed a commit that referenced this pull request Nov 5, 2024
adds a new `changed` event to `TreeBranchEvents` that fires for both
local and remote changes

---------

Co-authored-by: Noah Encke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: tree area: dds Issues related to distributed data structures area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct base: main PRs targeted against main branch changeset-present public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants