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

fix(client-presence): ISessionClient naming consistency #22973

Merged
merged 10 commits into from
Nov 4, 2024

Conversation

jason-ha
Copy link
Contributor

@jason-ha jason-ha commented Nov 4, 2024

  1. ISessionClient method names updated for consistency to getConnectionId() and getConnectionStatus().
  2. Implementation of ISessionClient moved to a full class object.
  3. Changeset provided for Presence changes since 2.4.
  4. Updated id to ID in comments (public and most internal).

No behavior is changed.

AB#21446

@jason-ha jason-ha requested a review from a team as a code owner November 4, 2024 22:04
@github-actions github-actions bot added area: examples Changes that focus on our examples 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 4, 2024
@jason-ha jason-ha added the release-blocking Must be addressed before we cut and publish the next release label Nov 4, 2024
@jason-ha
Copy link
Contributor Author

jason-ha commented Nov 4, 2024

Replaces #22889

Copy link
Member

@tylerbutler tylerbutler left a comment

Choose a reason for hiding this comment

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

I noticed a couple of lowercase id and ids in some of the surrounding comments. Probably not worth a further cleanup now. The opportunistic ones you did are great.

.changeset/ninety-zoos-trade.md Outdated Show resolved Hide resolved
.changeset/ninety-zoos-trade.md Outdated Show resolved Hide resolved
packages/framework/presence/src/presence.ts Outdated Show resolved Hide resolved

public setConnectionId(
connectionId: ClientConnectionId,
updateStatus: boolean = true,
Copy link
Member

Choose a reason for hiding this comment

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

When might you set updateStatus to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We think the management of connectivity is funky at the moment. I expect this will change when Task 22616: "Address ISessionClient connectivity consistency" is done. I hope this parameter just goes away. We'll see. I am using it now to preserve the logic as-is.

@jason-ha
Copy link
Contributor Author

jason-ha commented Nov 4, 2024

I noticed a couple of lowercase id and ids in some of the surrounding comments. Probably not worth a further cleanup now. The opportunistic ones you did are great.

I thought I got all in the files that were already touched and anything public facing.

@jason-ha jason-ha enabled auto-merge (squash) November 4, 2024 22:59
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.framework.presence.src:
Line Coverage Change: 0.21%    Branch Coverage Change: 0.18%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 83.87% 84.05% ↑ 0.18%
Line Coverage 61.93% 62.14% ↑ 0.21%

Baseline commit: 80ed028
Baseline build: 304016
Happy Coding!!

Code coverage comparison check passed!!

@msfluid-bot
Copy link
Collaborator

@fluid-example/bundle-size-tests: +245 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 464.41 KB 464.45 KB +35 Bytes
azureClient.js 562.65 KB 562.7 KB +49 Bytes
connectionState.js 724 Bytes 724 Bytes No change
containerRuntime.js 261.98 KB 261.99 KB +14 Bytes
fluidFramework.js 425.15 KB 425.16 KB +14 Bytes
loader.js 134.17 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.5 KB 528.55 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.75 KB 164.76 KB +7 Bytes
sharedTree.js 415.61 KB 415.62 KB +7 Bytes
Total Size 3.36 MB 3.36 MB +245 Bytes

Baseline commit: 80ed028

Generated by 🚫 dangerJS against 2431c54

@jason-ha jason-ha merged commit 6096657 into main Nov 4, 2024
32 checks passed
@jason-ha jason-ha deleted the presence/rename-status branch November 4, 2024 23:51
Josmithr pushed a commit that referenced this pull request Nov 5, 2024
1. `ISessionClient` method names updated for consistency to
`getConnectionId()` and `getConnectionStatus()`.
2. Implementation of `ISessionClient` moved to a full class object.
3. Changeset provided for Presence changes since 2.4.
4. Updated `id` to `ID` in comments (public and most internal).

No behavior is changed.

[AB#21446](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/21446)

---------

Co-authored-by: Willie Habimana <[email protected]>
Co-authored-by: Tyler Butler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: examples Changes that focus on our examples 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 release-blocking Must be addressed before we cut and publish the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants