-
Notifications
You must be signed in to change notification settings - Fork 536
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
Tiny cleanup of PureDataObject #23623
base: main
Are you sure you want to change the base?
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.
@@ -94,8 +94,6 @@ export abstract class PureDataObject<I extends DataObjectTypes = DataObjectTypes | |||
get IFluidHandle(): IFluidHandleInternal<this>; | |||
get IFluidLoadable(): this; | |||
initializeInternal(existing: boolean): Promise<void>; | |||
// (undocumented) | |||
protected initializeP: Promise<void> | undefined; |
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.
this is a breaking change to a legacy api, so will have to go through the breaking process which requires deprecation, and then removal in a .10 release
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.
We are breaking no documented behavior. Anything using this relies entirely on undocumented behavior.
I'm not clear on the exact details of what our policy on changing undocumented behavior, but I generally thought that this was something that's allowed, especially if there is no reasonable reason anyone would ever use it, a clear better documented alternative has always existed, and its removal isn't going to cause compile errors in any reasonable case (since this isn't an interface and if used as an interface this member would be omitted anyway since its protected).
We could leave a vestigial deprecated always undefined value on the object to ensure there is no difference in the type signature level. Would that be fine? It seems pointless but would cause this change to not appear in our API reports. That doesn't seem better to me but maybe its allowed by whatever our policy is here?
It seems more likely that introducing the private initializationPromise property is a breaking change since that can cause a name collision and runtime breakage in real code not relying on undocumented behavior, but we just ignore those kinds of changes and be overly strict about the easy to nit pick stuff in API reports?
I think we need a relatively flexible definition of what is breaking driven based on documented behavior and what people might reasonably end up using, at least for any usage of non-sealed classes in the API surface: subclassing is real fragile and the API reports don't accurately capture what is actually risky and what is safe.
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 just moved initializationPromise to use a JS private property to avoid that being a possible break (might as well, once we move to es2022 that pattern will get better minification too). No need to include an additional possible break.
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.
we generally take a conservative view of all breaking changes, as before all breaks we try to ensure there is no usage before hand and that an alternative exists. Only then do we deprecate, then remove. this ensures consistency for our legacy partners in that they can safely take all non-.10 minor and there should be minimal chance of issues, and we batch all changes that could cause issues in our .10 release so partners can craft semvers, and plan around these possibly more disruptive releases. @jason-ha could probably provide more guidance.
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.
Tony is correct here. This falls within our legacy+alpha SLA and should be staged.
https://github.com/microsoft/FluidFramework/wiki/Breaking-vs-Non-breaking-Changes#breaking
If there are no partners using this, then there is an opportunity for accelerated removal in 2.30, otherwise 2.40. Follow the wiki guidance and thanks for cleaning up!
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 have removed this change. The value gained from it is too small to be worth the full process.
if (this.initializeP !== undefined) { | ||
return this.initializeP; | ||
} | ||
this.initializeP = this.initializeInternal(existing); | ||
return this.initializeP; | ||
this.#initializationPromise ??= this.initializeInternal(existing); | ||
return this.#initializationPromise; |
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.
While undocumented and unclear, the conservative change would be to continue assigning this.initializationP
here.
this.initializationP = this.#initializationPromise;
looks like it would preserve behavior.
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.
That would not preserve all the existing behavior. This property is mutable, and subclasses could assign to it to cause finishInitialization to return a different promise, skip initialization, or clear it to cause finishInitialization to run again next time its called.
I don't see why anyone would do any of those things, but the same applies for reading this field: it's just not a useful property to use, and none of the possible uses are specified.
Anyway, I have removed the rename, keeping all these undocumented behaviors the same. I don't think it's worth the trouble of going through a deprecation here: this can just go away when we get around to deprecating the entire class (I don't think subclassing based data object definition which forces use of a directory is something we will be keeping long term, especially given active work to implement and migrate to alternatives)
🔗 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.
Please update title and description to match the current version of changes.
* Call this API to ensure PureDataObject is fully initialized. | ||
* Await this API to ensure PureDataObject is fully initialized. | ||
* Initialization happens on demand, only on as-needed bases. | ||
* In most cases you should allow factory/object to decide when to finish initialization. | ||
* But if you are supplying your own implementation of DataStoreRuntime factory and overriding some methods | ||
* and need a fully initialized object, then you can call this API to ensure object is fully initialized. |
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.
Probably fine to leave. But "call ... to ensure" also appears on line 131, rather than the updated "await" text.
Description
Tiny cleanup of PureDataObject.
Breaking Changes
The undocumented protected property
PureDataObject.initializeP
is no longer exposed.This removal does not break any documented behavior and there was no clear useful way to use this property even if relying on undocumented behavior.
finishInitialization
exists and is documented to allow any synchronization that could reasonably be done using initializeP.Reviewer Guidance
The review process is outlined on this wiki page.