feat(client-fluid-framework): expose getPresence from fluid-framework#26399
feat(client-fluid-framework): expose getPresence from fluid-framework#26399jason-ha wants to merge 6 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR exposes getPresence / getPresenceAlpha from the fluid-framework package and deprecates importing getPresence directly from @fluidframework/presence, updating docs/examples accordingly.
Changes:
- Re-export
getPresenceandgetPresenceAlphafromfluid-framework(beta/alpha surfaces). - Deprecate
@fluidframework/presence’sgetPresenceandgetPresenceAlphaexports and update API reports. - Update documentation/examples to use
import { getPresence } from "fluid-framework/beta".
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds workspace dependency entry for @fluidframework/presence where needed. |
| packages/service-clients/end-to-end-tests/azure-client/src/test/multiprocess/childClient.tool.ts | Adds deprecation lint suppressions around getPresence usage. |
| packages/framework/presence/src/getPresence.ts | Marks getPresence / getPresenceAlpha as deprecated and adjusts imports/aliases. |
| packages/framework/presence/api-report/presence.legacy.alpha.api.md | Marks getPresence / getPresenceAlpha as deprecated in legacy alpha report. |
| packages/framework/presence/api-report/presence.beta.api.md | Marks getPresence as deprecated in beta report. |
| packages/framework/presence/api-report/presence.alpha.api.md | Marks getPresence / getPresenceAlpha as deprecated in alpha report. |
| packages/framework/presence/README.md | Updates usage snippet to import getPresence from fluid-framework/beta. |
| packages/framework/fluid-framework/src/index.ts | Adds custom re-exports for presence getters from @fluidframework/presence/alpha. |
| packages/framework/fluid-framework/package.json | Adds @fluidframework/presence dependency. |
| packages/framework/fluid-framework/eslint.config.mts | Disables import-x/order for src/index.ts. |
| packages/framework/fluid-framework/api-report/fluid-framework.legacy.beta.api.md | Adds getPresence to legacy beta API report. |
| packages/framework/fluid-framework/api-report/fluid-framework.beta.api.md | Adds getPresence to beta API report. |
| packages/framework/fluid-framework/api-report/fluid-framework.alpha.api.md | Adds getPresence / getPresenceAlpha to alpha API report. |
| examples/service-clients/azure-client/external-controller/src/app.ts | Migrates example import to fluid-framework/beta. |
| examples/apps/presence-tracker/src/app.ts | Migrates getPresence import to fluid-framework/beta while keeping presence types in @fluidframework/presence. |
| docs/docs/build/presence.mdx | Updates docs snippet to import getPresence from fluid-framework/beta. |
| .changeset/beige-candles-sleep.md | Adds deprecation-focused changeset describing the migration pattern. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
...ages/service-clients/end-to-end-tests/azure-client/src/test/multiprocess/childClient.tool.ts
Outdated
Show resolved
Hide resolved
.changeset/beige-candles-sleep.md
Outdated
| "@fluidframework/presence": minor | ||
| "__section": deprecation | ||
| --- | ||
| getPresence is being relocated to fluid-framework package away from @fluidframework/presence |
There was a problem hiding this comment.
Minor wording/grammar: consider rephrasing to "getPresence is being relocated to the fluid-framework package, away from @fluidframework/presence." (adds articles/backticks and reads more cleanly in the changeset).
| getPresence is being relocated to fluid-framework package away from @fluidframework/presence | |
| getPresence is being relocated to the `fluid-framework` package, away from `@fluidframework/presence`. |
There was a problem hiding this comment.
I had those tick but removed them. I thought we didn't want ticks in the title (for reason I didn't question).?
There was a problem hiding this comment.
Yeah, our current guidance is to not use inline code syntax in changeset titles to prevent noisy looking headers in the resulting changeset. I think this is a policy we could discuss as a team if folks feel strongly that we should allow them (I have mixed feelings personally). But for now, the guidance is no.
There was a problem hiding this comment.
What Josh said, and I'll add that there shouldn't be a trailing period.
There was a problem hiding this comment.
The one reason I know to allow them is to avoid the @ mentions that happen without them. We've mentioned @alpha and @beta in the past. Or maybe we've fixed that since.?
There was a problem hiding this comment.
Oh, interesting. I wasn't aware of that issue. You can always escape the @ if needbe though. At least I would assume that works.
There was a problem hiding this comment.
I don't think the issue is fixed. That's a minor reason why I personally don't mind the backticks, but not a battle I care about fighting 😄 . Somehow trying to escape the @ had never occurred to me... it does work when used directly in comment boxes here (the picker doesn't come up). Hopefully it would work in the rendered release notes.
There was a problem hiding this comment.
I have added ticks since the mention issue isn't addressed.
| import type { ScopeType } from "@fluidframework/driver-definitions/legacy"; | ||
| import type { ContainerSchema, IFluidContainer } from "@fluidframework/fluid-static"; | ||
| import { | ||
| // eslint-disable-next-line import-x/no-deprecated -- TODO#59157: relocating to fluid-static |
There was a problem hiding this comment.
The TODO in this eslint suppression says relocating to fluid-static, but this PR is relocating getPresence to fluid-framework (and the deprecation text in @fluidframework/presence points to fluid-framework/beta). Please update the TODO text to reflect the actual intended destination (or link the correct tracking issue if it really is moving to fluid-static).
| // eslint-disable-next-line import-x/no-deprecated -- TODO#59157: relocating to fluid-static | |
| // eslint-disable-next-line import-x/no-deprecated -- TODO#59157: relocating to fluid-framework/beta |
There was a problem hiding this comment.
Do we need the export in fluid-static? Users of azure-client are expected to also depend on fluid-framework, no? So they could get the API from there.
There was a problem hiding this comment.
I need to relocate it outside of the presence package. fluid-static is the best home I can think of. Otherwise, I will create a new package. fluid-framework can continue to have the export but it will import from fluid-static instead.
I can't move getPresence straight to fluid-static because presence currently depends on fluid-static. There will be presence package refactor to do the other shuffling. But in order to deprecate it, I need a replacement. Hence this staging.
There was a problem hiding this comment.
presence has been refactored and now getPresence is implemented in fluid-static.
There was a problem hiding this comment.
@Josmithr, fluid-static is the package that pulls together the "encapsulated" layers and enables the "declarative" use pattern. In particular, it implements FluidContainer that is the host for Presence connection as it is for DDSes.
I am guessing that IFluidContainer may get extended to just have a presence member. But for now, we let customers demand Presence via getPresence but are placing that in the vicinity.
If presence is the general Presence package, it must not pull in "encapsulated" or "declarative" specific things. The getPresence* access functions violated that. I am not sure where the "encapsulated" accessor will land, but that is probably a new (small) package.
alexvy86
left a comment
There was a problem hiding this comment.
Some doc-related comments
| import type { ScopeType } from "@fluidframework/driver-definitions/legacy"; | ||
| import type { ContainerSchema, IFluidContainer } from "@fluidframework/fluid-static"; | ||
| import { | ||
| // eslint-disable-next-line import-x/no-deprecated -- TODO#59157: relocating to fluid-static |
There was a problem hiding this comment.
Do we need the export in fluid-static? Users of azure-client are expected to also depend on fluid-framework, no? So they could get the API from there.
- Move support to `@fluidframework/fluid-static`, re-export from `fluid-framework`, and deprecate from `@fluidframework/presence` - Update fluid-static exports to support alpha and beta - `fluid-framework` only exports `getPresence*`. Callers can access remaining surface from `@fluidframework/presence`. - Update example and test uses
feb2589 to
80f71a9
Compare
| --- | ||
| getPresence is being relocated from @fluidframework/presence to the fluid-framework package | ||
|
|
||
| To prepare, make changes following this pattern: |
There was a problem hiding this comment.
Nit: I think we should expand this a bit. Something like "The export from @fluidframework/presence is now deprecated and will be removed in a future release. Please update any existing imports as follows:"
Josmithr
left a comment
There was a problem hiding this comment.
I think I'm missing some context on the changes here. The user-facing aspects of this look good to me, and the docs look good (aside from 1 nitpick).
But the move to fluid-static is a bit surprising. The scope / intention of that package isn't well documented, but I always viewed it as the APIs needed to implement a service client. Since the service clients don't use presence (right?), that placement seems a bit weird to me.
I'm assuming there were some external conversations that led to this outcome?
I would look at this thread: #26399 (comment). I will post extra notes there. |
add blank line after title
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
@fluidframework/fluid-static, re-export fromfluid-framework, and deprecate from@fluidframework/presencefluid-frameworkonly exportsgetPresence*. Callers can access remaining surface from@fluidframework/presence.