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

Test/internal merge tree client #22697

Draft
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

anthony-murphy
Copy link
Contributor

@anthony-murphy anthony-murphy commented Oct 1, 2024

in progress

@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: sharedstring area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: tests Tests to add, test infrastructure improvements, etc public api change Changes to a public API base: main PRs targeted against main branch labels Oct 1, 2024
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Oct 1, 2024

@fluid-example/bundle-size-tests: -165 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 464.08 KB 463.91 KB -170 Bytes
azureClient.js 562.24 KB 562.29 KB +49 Bytes
connectionState.js 724 Bytes 724 Bytes No change
containerRuntime.js 261.67 KB 261.68 KB +14 Bytes
fluidFramework.js 406.75 KB 406.77 KB +14 Bytes
loader.js 134.16 KB 134.18 KB +14 Bytes
map.js 42.71 KB 42.71 KB +7 Bytes
matrix.js 148.54 KB 148.54 KB +7 Bytes
odspClient.js 528.18 KB 528.23 KB +49 Bytes
odspDriver.js 97.84 KB 97.86 KB +21 Bytes
odspPrefetchSnapshot.js 42.81 KB 42.82 KB +14 Bytes
sharedString.js 164.73 KB 164.54 KB -198 Bytes
sharedTree.js 397.21 KB 397.22 KB +7 Bytes
Total Size 3.32 MB 3.32 MB -165 Bytes

Baseline commit: 30003a8

Generated by 🚫 dangerJS against a898922

@anthony-murphy
Copy link
Contributor Author

anthony-murphy commented Oct 16, 2024

#22827

@@ -193,6 +193,7 @@
},
"typeValidation": {
"broken": {},
"disabled": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why full stop disable? Isn't this exactly the case where we want to list the incompatibilities? Is another option to change entrypoint from "legacy" to "public"?

Copy link
Contributor Author

@anthony-murphy anthony-murphy Oct 18, 2024

Choose a reason for hiding this comment

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

things like the 2.4 release and other changes create lots of merge conflicts. i don't want to deal with merge conflict right now. once we are closer to the merge point i'll remove it and deal with them

Copy link
Contributor

@jason-ha jason-ha left a comment

Choose a reason for hiding this comment

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

Shouldn't you be able to make many of the test changes before the breaking change?
When we have a well staged deprecation, I would expect to see more removal of things we don't have to test for anymore compared to test adaptation. The latter suggests customers need to adapt too. OR we don't have good coverage on the external facing APIs which also hinders confidence.

packages/dds/sequence/src/intervalCollection.ts Outdated Show resolved Hide resolved
Comment on lines 40 to 48
import {
IIntervalHelpers,
ISerializableInterval,
ISerializedInterval,
IntervalStickiness,
IntervalType,
endReferenceSlidingPreference,
startReferenceSlidingPreference,
type IIntervalHelpers,
} from "./intervalUtils.js";
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the sorting would still want type IIntervalHelpers first.
Other pattern that has no such confusion is to have separate import type {} block. We do that more now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't know. i just let the formatter do what it wants.

readonly stickiness: IntervalStickiness;
}

export class SequenceIntervalClass implements SequenceInterval {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have an explicit tsdoc and @internal tag. No complaints from the tooling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no it shouldn't, it's never exported from the package

@anthony-murphy
Copy link
Contributor Author

Shouldn't you be able to make many of the test changes before the breaking change? When we have a well staged deprecation, I would expect to see more removal of things we don't have to test for anymore compared to test adaptation. The latter suggests customers need to adapt too. OR we don't have good coverage on the external facing APIs which also hinders confidence.

that is not possible given the changes. the changes literally rename the class and make it un-exported

@jason-ha
Copy link
Contributor

Just having another staging thought. For the cases where you now have a FooClass and Foo is interface, I think you can currently do
const FooClass = Foo;
exporting as needed for internal use. Then there is better clarity where you need to preserve the class internally versus interface and less churn when the time comes.
Unfortunately, I don't know that we can get away with the reverse (const Foo = FooClass;) at least not without more song and dance per api-extractor.

@anthony-murphy
Copy link
Contributor Author

anthony-murphy commented Oct 18, 2024

Just having another staging thought. For the cases where you now have a FooClass and Foo is interface, I think you can currently do const FooClass = Foo; exporting as needed for internal use. Then there is better clarity where you need to preserve the class internally versus interface and less churn when the time comes. Unfortunately, I don't know that we can get away with the reverse (const Foo = FooClass;) at least not without more song and dance per api-extractor.

i don't understand this comment. i'm not exporting anything concrete. classes are being replaced with interfaces, nothing but types are exported

@jason-ha
Copy link
Contributor

Shouldn't you be able to make many of the test changes before the breaking change? When we have a well staged deprecation, I would expect to see more removal of things we don't have to test for anymore compared to test adaptation. The latter suggests customers need to adapt too. OR we don't have good coverage on the external facing APIs which also hinders confidence.

that is not possible given the changes. the changes literally rename the class and make it un-exported

I am not sure I follow. Either you are doing non-"public" testing and still internally "export"ing the class or you shouldn't need to access the class beyond its instance type (no ctor) for testing.

@anthony-murphy
Copy link
Contributor Author

anthony-murphy commented Oct 18, 2024

Shouldn't you be able to make many of the test changes before the breaking change? When we have a well staged deprecation, I would expect to see more removal of things we don't have to test for anymore compared to test adaptation. The latter suggests customers need to adapt too. OR we don't have good coverage on the external facing APIs which also hinders confidence.

that is not possible given the changes. the changes literally rename the class and make it un-exported

I am not sure I follow. Either you are doing non-"public" testing and still internally "export"ing the class or you shouldn't need to access the class beyond its instance type (no ctor) for testing.

the test's are for implementations, they unit test the actual class which is only ever constructed within the package, consumers only ever see the interface. The tests pay the cost of dealing with the rename, and the interface get the original name, so customers don't have to change anything.

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.merge-tree.src:
Line Coverage Change: -0.01%    Branch Coverage Change: -0.01%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 94.90% 94.89% ↓ -0.01%
Line Coverage 97.08% 97.07% ↓ -0.01%
↑ packages.dds.sequence.src:
Line Coverage Change: 0.07%    Branch Coverage Change: -0.05%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 89.47% 89.42% ↓ -0.05%
Line Coverage 89.74% 89.81% ↑ 0.07%
↑ packages.dds.sequence.src.intervalIndex:
Line Coverage Change: 0.03%    Branch Coverage Change: No change
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 92.55% 92.55% → No change
Line Coverage 87.31% 87.34% ↑ 0.03%
↑ packages.dds.sequence.src.intervals:
Line Coverage Change: 0.03%    Branch Coverage Change: No change
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 89.47% 89.47% → No change
Line Coverage 96.22% 96.25% ↑ 0.03%

Baseline commit: 30003a8
Baseline build: 303240
Happy Coding!!

Code coverage comparison check passed!!

Copy link
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:linkcheck /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test ci:start 1313 linkcheck:full

1: starting server using command "npm run ci:start"
and when url "[ 'http://127.0.0.1:1313' ]" is responding with HTTP status code 200
running tests using command "npm run linkcheck:full"


> [email protected] ci:start
> http-server ./public --port 1313 --silent


> [email protected] linkcheck:full
> npm run linkcheck:fast -- --external


> [email protected] linkcheck:fast
> linkcheck http://localhost:1313 --skip-file skipped-urls.txt --external

Crawling...

Stats:
  440714 links
    3397 destination URLs
       2 URLs ignored
       0 warnings
       0 errors


@@ -87,104 +78,6 @@ export abstract class BaseSegment implements ISegment {
wasMovedOnInsert?: boolean | undefined;
}

// @alpha @deprecated (undocumented)
export class Client extends TypedEventEmitter<IClientEvents> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tylerbutler do you know why these type removals don't break the type tests? i would expect those to need suppression, but they don't for some reason.

Copy link
Contributor Author

@anthony-murphy anthony-murphy Oct 30, 2024

Choose a reason for hiding this comment

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

i guess it makes sense if the look at the imports

import type * as old from "@fluidframework/merge-tree-previous/internal";

import type * as current from "../../index.js";

as the types still exist off internal, just not legacy, so are still available off import type * as current from "../../index.js";

however, i would expect this to be a breaking change, as something that was exported at one layer, is no longer exported.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • @CraigMacomber as type test collaborator.
    I think there was a task out there to test against the entrypoints directly. I think that may have been mutated to read the tags so that all pulled into a single file without any duplication. I still think that using the entrypoints directly has merit. Especially relevant if we hand craft any entrypoints, which would allow deviation from tags.

Copy link
Contributor

@jason-ha jason-ha left a comment

Choose a reason for hiding this comment

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

I looked over this more and tried to separate more work that can be done right now as I had been mentioning in other comments. From that I produced #22948. Have a look.
I also have the remainder of the changes in a stash. Final result is not identical to this as I made some fixes.
I think the ideal process for these overall changes are:

  1. improvement(client): deprecation cleanup #22948
  2. just breaks (blocked on 2.10 opening)
  3. more taking advantage of the breaks (change internals)

The healthiest way to maintain 2 & 3 over time is likely to be having branch with two commits and rebasing over main when desired.

packages/dds/sequence/src/sequenceDeltaEvent.ts Outdated Show resolved Hide resolved
packages/dds/sequence/src/sequenceDeltaEvent.ts Outdated Show resolved Hide resolved
@@ -87,104 +78,6 @@ export abstract class BaseSegment implements ISegment {
wasMovedOnInsert?: boolean | undefined;
}

// @alpha @deprecated (undocumented)
export class Client extends TypedEventEmitter<IClientEvents> {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • @CraigMacomber as type test collaborator.
    I think there was a task out there to test against the entrypoints directly. I think that may have been mutated to read the tags so that all pulled into a single file without any duplication. I still think that using the entrypoints directly has merit. Especially relevant if we hand craft any entrypoints, which would allow deviation from tags.

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: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants