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

Remove convertToSummaryTreeWithStats from DDSs #8737

Merged

Conversation

scarlettjlee
Copy link
Contributor

Change remaining summarizeCore implementations to directly create ISummaryTreeWithStats instead of creating ITree and then converting. This completes the work started in #8592 which changed the return type from ITree to ISummaryTreeWithStats.

There are uses of conversion functions in container runtime (will be fixed with #8286), summarizer node, and data stores so I can't remove them yet.

I'll remove the fullTree parameter in summarizeCore in a separate PR to keep this change cleaner.

Closes #8652

@scarlettjlee scarlettjlee added this to the January 2022 milestone Jan 13, 2022
@scarlettjlee scarlettjlee requested a review from a team as a code owner January 13, 2022 09:46
@scarlettjlee scarlettjlee self-assigned this Jan 13, 2022
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: propertydds area: dds: sharedstring breaking change This PR or issue would introduce a breaking change public api change Changes to a public API labels Jan 13, 2022
@github-actions github-actions bot requested review from vladsud and anthony-murphy and removed request for a team January 13, 2022 09:46
@msfluid-bot
Copy link
Collaborator

@fluid-example/bundle-size-tests: -2.26 KB
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 380.69 KB 380.14 KB -566 Bytes
containerRuntime.js 179.55 KB 179.36 KB -194 Bytes
loader.js 160.56 KB 160.56 KB No change
map.js 51.18 KB 50.99 KB -194 Bytes
matrix.js 146.13 KB 145.51 KB -632 Bytes
odspDriver.js 190.9 KB 190.75 KB -157 Bytes
odspPrefetchSnapshot.js 44.69 KB 44.69 KB No change
sharedString.js 167.1 KB 166.55 KB -567 Bytes
Total Size 1.32 MB 1.32 MB -2.26 KB

Baseline commit: 90b900a

Generated by 🚫 dangerJS against 8291570

Copy link
Contributor

@anthony-murphy anthony-murphy left a comment

Choose a reason for hiding this comment

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

This is awesome. So happy to see this getting cleaned up.

Copy link
Contributor

@vladsud vladsud left a comment

Choose a reason for hiding this comment

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

It's easy to make mistake here and not notice.
Can you please validate that each case you touched

  • has UT covering summarization
  • OR covered in snapshot tests (i.e. there is file that touches that DDS)
  • OR we validated summarization manually (by running some sample that uses it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: propertydds area: dds: sharedstring area: dds Issues related to distributed data structures breaking change This PR or issue would introduce a breaking change public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove ITree-related code from DDSs
4 participants