-
Notifications
You must be signed in to change notification settings - Fork 537
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
(op bunching) Deprecate process on FluidDataStoreRuntime #23866
(op bunching) Deprecate process on FluidDataStoreRuntime #23866
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
packages/runtime/datastore/src/dataStoreRuntime.ts:797
- The
default
case in theswitch
statement should handle unrecognized message types to avoid silent failures. Consider adding a log or throwing an error.
default:
"@fluidframework/datastore": minor | ||
--- | ||
--- | ||
"section": deprecation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not saying this is necessarily correct but I wonder if we should put it in the section for changes to the legacy API since that's where FluidDataStoreRuntime
lives. cc @tylerbutler and @jason-ha for thoughts. I know we used deprecation
in the previous change so maybe that's reason enough to have it match here, but at least wanted to bring it up for discussion which might affect future deprecations of legacy APIs.
"section": deprecation | |
"section": legacy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong feeling either way. For now, I am keeping it deprecation to be in line with the previous change like you said.
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking merge only to help with release race-conditions. I'll remove this block once release branch is created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API Approval
…3866) Deprecating `process` on `FluidDataStoreRuntime`. It was deprecated on `IFluidDataStoreChannel` (and other places) as part of microsoft#22840 but missed deprecating it on `FluidDataStoreRuntime`.
Deprecating
process
onFluidDataStoreRuntime
. It was deprecated onIFluidDataStoreChannel
(and other places) as part of #22840 but missed deprecating it onFluidDataStoreRuntime
.