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.

Sign up for GitHub

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

Jump to bottom

Remove the exposure of SharedDirectory API #20030

Merged
merged 20 commits into from
Mar 27, 2024
Merged

Remove the exposure of SharedDirectory API #20030

merged 20 commits into from
Mar 27, 2024

Conversation

clarenceli-msft
Copy link
Contributor

@clarenceli-msft clarenceli-msft commented Mar 8, 2024 •
edited
Loading

Hide the class implementation of SharedDirectory and update the corresponding exports, a follow up fix of #19717

AB#5799

@clarenceli-msft clarenceli-msft marked this pull request as draft March 8, 2024 17:14
@github-actions github-actions bot added area: dds Issues related to distributed data structures public api change Changes to a public API base: main PRs targeted against main branch labels Mar 8, 2024
@clarenceli-msft clarenceli-msft changed the title [Draft] Remove the exposure of SharedDirectory [Draft] Remove the exposure of SharedDirectory API Mar 8, 2024
@github-actions github-actions bot added area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: tests Tests to add, test infrastructure improvements, etc labels Mar 8, 2024
@clarenceli-msft clarenceli-msft changed the title [Draft] Remove the exposure of SharedDirectory API Remove the exposure of SharedDirectory API Mar 8, 2024
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Mar 8, 2024 •
edited
Loading

⯆ @fluid-example/bundle-size-tests: -156 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 520.41 KB 520.36 KB ⯆ -52 Bytes
azureClient.js 611.54 KB 611.49 KB ⯆ -52 Bytes
connectionState.js 680 Bytes 680 Bytes ■ No change
containerRuntime.js 254.6 KB 254.6 KB ■ No change
fluidFramework.js 341.46 KB 341.46 KB ■ No change
loader.js 127.97 KB 127.97 KB ■ No change
map.js 41.35 KB 41.35 KB ■ No change
matrix.js 143.61 KB 143.61 KB ■ No change
odspClient.js 580 KB 579.95 KB ⯆ -52 Bytes
odspDriver.js 97.49 KB 97.49 KB ■ No change
odspPrefetchSnapshot.js 41.91 KB 41.91 KB ■ No change
sharedString.js 161.38 KB 161.38 KB ■ No change
sharedTree.js 331.59 KB 331.59 KB ■ No change
Total Size 3.33 MB 3.32 MB ⯆ -156 Bytes

Baseline commit: f27289d

Generated by 🚫 dangerJS against 74a4352

@clarenceli-msft clarenceli-msft marked this pull request as ready for review March 8, 2024 21:02
@clarenceli-msft clarenceli-msft requested a review from a team as a code owner March 8, 2024 21:02
CraigMacomber
CraigMacomber reviewed Mar 8, 2024
View reviewed changes
packages/test/test-end-to-end-tests/src/test/directoryEndToEndTests.spec.ts
@@ -1027,7 +1027,7 @@ describeCompat("SharedDirectory orderSequentially", "NoCompat", (getTestObjectPr
assert.notEqual(error, undefined, "No error");
assert.equal(error?.message, errorMessage, "Unexpected error message");
assert.equal(containerRuntime.disposed, false);
assert.equal(sharedDir.countSubDirectory(), 0);
assert.equal(sharedDir.countSubDirectory?.(), 0);
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 the only implementation of ISharedDirectory is SharedDirectory, so it seems like this methods should be able to be made non-optional on it, allowing these tests (and possible apps using it as well) to remain unchanined.

Copy link
Contributor

Choose a reason for hiding this comment

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

might be good to try and figure out why that method was made optional in the first place as its doc's don't say. Maybe there are third parties implementing the interface that can't implement that?

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 reviewed the update history of this method, and it appears that the FFX repo did implement the IDirectory interface. Somehow we made this method optional. https://github.com/microsoft/FluidFramework/pull/9766/files#r844412897
Add @jatgarg here

@msfluid-bot msfluid-bot added the size regression Significant bundle size regression (>5 KB) label Mar 11, 2024
Abe27342
Abe27342 reviewed Mar 21, 2024
View reviewed changes
Copy link
Contributor

@Abe27342 Abe27342 left a comment

Choose a reason for hiding this comment

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

looks good, mostly some organizational nits. We'll need to do some merging with concepts added in #20028. So keep an eye on that before merging.

Abe27342
Abe27342 reviewed Mar 26, 2024
View reviewed changes
Copy link
Contributor

@Abe27342 Abe27342 left a comment

Choose a reason for hiding this comment

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

Can ISerializableValue and ISerializedValue be removed from the public API as well? Doesn't look like they're referenced anymore.

@clarenceli-msft
Copy link
Contributor Author

Can ISerializableValue and ISerializedValue be removed from the public API as well? Doesn't look like they're referenced anymore.

Moved them to ./internalInterface.ts and removed the exposure of them from API

@clarenceli-msft clarenceli-msft enabled auto-merge (squash) March 26, 2024 17:50
alexvy86
alexvy86 approved these changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

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

Devtools change is good. Left a general comment about the changeset though.

.changeset/clever-cats-jump.md
Comment on lines +2 to +3
"fluid-framework": minor
"@fluidframework/map": minor
Copy link
Contributor

Choose a reason for hiding this comment

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

There was a discussion about this on another PR, I'm on the side that we should try to be deliberate about this bit in a changeset. What we're doing here qualifies as a major change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops it is automatically merged in, I will create a small PR to update it.

@clarenceli-msft clarenceli-msft merged commit 2e699b8 into microsoft:main Mar 27, 2024
31 checks passed
@clarenceli-msft clarenceli-msft deleted the remove-exposure-directory-api branch March 27, 2024 20:20
@clarenceli-msft clarenceli-msft mentioned this pull request Mar 27, 2024
Merged
clarenceli-msft added a commit that referenced this pull request Mar 27, 2024
@clarenceli-msft
Update changeset about map/directory API cleaning (#20366)
31dbb22 
A follow-up update of
#20030
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Abe27342 Abe27342 Abe27342 approved these changes

@alexvy86 alexvy86 alexvy86 approved these changes

@Josmithr Josmithr Awaiting requested review from Josmithr

@CraigMacomber CraigMacomber Awaiting requested review from CraigMacomber

@scarlettjlee scarlettjlee Awaiting requested review from scarlettjlee

Assignees
No one assigned
Labels
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 size regression Significant bundle size regression (>5 KB)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@clarenceli-msft @msfluid-bot @alexvy86 @Abe27342 @CraigMacomber @scarlettjlee