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

chore(merge-tree sequence examples): remove NestBegin and NestEnd reference types #17546

Merged

Conversation

connorskees
Copy link
Member

This is a ~conservative first pass at removing some of the tech debt related to the NestBegin and NestEnd reference types. Alongside these enum variants, this change removes getStackContext, RangeStackMap, the shared-text example, and a few other things.

I will add some review comments to call out some changes (or lack thereof) to make sure they make sense once CI is green.

@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: sharedstring area: examples Changes that focus on our examples public api change Changes to a public API base: next PRs targeted against next branch labels Sep 29, 2023
@github-actions github-actions bot added the area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct label Sep 29, 2023
api-report/merge-tree.api.md Outdated Show resolved Hide resolved
api-report/merge-tree.api.md Outdated Show resolved Hide resolved
api-report/merge-tree.api.md Outdated Show resolved Hide resolved
api-report/merge-tree.api.md Outdated Show resolved Hide resolved
Copy link
Member Author

Choose a reason for hiding this comment

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

For prosemirror, the example seems good enough if we just do some finagling with the reference types. That said, it's not clear to me which reference type these should switch to, and the APIs being used appear quite old. I'm not sure if we want to remove this example entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

shared-text uses a lot of merge-tree internals, and it seems best just to delete entirely.

examples/data-objects/webflow/src/clipboard/paste.ts Outdated Show resolved Hide resolved
Comment on lines +352 to 353
if (refHasTileLabels(lref)) {
this.hierRefCount++;
Copy link
Member Author

Choose a reason for hiding this comment

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

I kept hierRefCount and the tiles stuff around as I wasn't sure if they were safe to remove.

}

public hierBlock() {
return this;
}

public hierToString(indentCount: number) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This didn't seem to have any usage anywhere

Copy link
Member Author

Choose a reason for hiding this comment

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

See also: 9dc1859

i'm ~intending to revert this change, but for now the webflow build
seems quite broken and i want to see the test run results in CI
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Sep 29, 2023

@fluid-example/bundle-size-tests: -5.9 KB
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 443.96 KB 441.95 KB -2.01 KB
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 237.29 KB 237.29 KB No change
loader.js 148.39 KB 148.39 KB No change
map.js 48.43 KB 48.43 KB No change
matrix.js 142.21 KB 140.32 KB -1.89 KB
odspDriver.js 90.46 KB 90.46 KB No change
odspPrefetchSnapshot.js 42.13 KB 42.13 KB No change
sharedString.js 162.86 KB 160.87 KB -2 KB
sharedTree2.js 258.88 KB 258.88 KB No change
Total Size 1.68 MB 1.67 MB -5.9 KB

Baseline commit: 550e869

Generated by 🚫 dangerJS against 57f94c5

anthony-murphy
anthony-murphy previously approved these changes Sep 29, 2023
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.

we need to do some deprecations in main for completeness, but i highly doubt any of this is in use. we also need a changeset here to call out the removals in mergetree/seq, keep it brief, as again, i don't think its used, as this stuff probably doesn't work anyway.

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.

actually, can we bring back web flow and test farm, feel free to rip features out, but i'd like to keep both. might make sense to do this with the deprecations in main

@anthony-murphy anthony-murphy dismissed their stale review September 29, 2023 17:44

a couple changes

@connorskees
Copy link
Member Author

testFarm wasn't deleted in this PR, it just has a lot of lines deleted. I did delete it in #17547, however.

@connorskees connorskees requested a review from a team as a code owner September 29, 2023 18:08
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Sep 29, 2023
@connorskees
Copy link
Member Author

Deprecation PR: #17555

@@ -577,8 +577,7 @@ export function createSequenceInterval(
endRefType = ReferenceType.Transient;
} else {
if (intervalType === IntervalType.Nest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should also deprecate IntervalType.Nest

connorskees added a commit that referenced this pull request Sep 29, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2023

This PR is ready to merge! Please review it and squash merge into next: @sonalideshpandemsft @tylerbutler @scottn12

@tylerbutler tylerbutler merged commit c426adf into microsoft:next Oct 2, 2023
33 checks passed
@connorskees connorskees deleted the chore/remove-nest-reference-types branch October 13, 2023 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: sharedstring area: dds Issues related to distributed data structures area: examples Changes that focus on our examples area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct base: next PRs targeted against next branch dependencies Pull requests that update a dependency file msftbot: merge-next public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants