Remove extra calls to bind when not attached #21213
Conversation
github-actions
bot
added
area: dds
| @@ -7,7 +7,7 @@ import type { TypedEventEmitter } from "@fluid-internal/client-utils"; | |||
| import type { IFluidHandle } from "@fluidframework/core-interfaces"; | |||
| import { assert, unreachableCase } from "@fluidframework/core-utils/internal"; | |||
| import type { IFluidSerializer } from "@fluidframework/shared-object-base/internal"; | |||
| import { ValueType, bindHandles } from "@fluidframework/shared-object-base/internal"; | |||
There was a problem hiding this comment.
You can also delete this function, it's not used anywhere else, and isn't an ideal implementation. @Abe27342 what's the task you pinged me about fixing the function? We can close it if this PR works out.
| @@ -84,8 +80,6 @@ export interface IMapDataObjectSerialized { | |||
| type MapKeyLocalOpMetadata = IMapKeyEditLocalOpMetadata | IMapKeyAddLocalOpMetadata; | |||
| type MapLocalOpMetadata = IMapClearLocalOpMetadata | MapKeyLocalOpMetadata; | |||
|
|
|||
| /* eslint-disable @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access */ | |||
There was a problem hiding this comment.
Do we know why these aren't necessary anymore?
There was a problem hiding this comment.
I think they were unrelated, but I have format on save turned on and it removed a bunch of lints that were no longer used apparently.
| @@ -147,6 +147,11 @@ | |||
| "typescript": "~5.3.3" | |||
| }, | |||
| "typeValidation": { | |||
| "broken": {} | |||
| "broken": { | |||
| "RemovedFunctionDeclaration_bindHandles": { | |||
There was a problem hiding this comment.
Why is this running on @internal things? Oh well.
There was a problem hiding this comment.
I thought this seemed odd too, but it removed the entries from validateSharedObjectPrevious which seemed like the right thing to do.
|
Fixes AB#7316 |
| const localValue = this.directory.localValueMaker.fromInMemory(value); | ||
| const serializableValue = makeSerializable( |
There was a problem hiding this comment.
@markfields I actually don't think this can be removed - without this, the visibility of the datastores is not getting set correctly. This also predated #19242, so I would think it's ok to leave.
There was a problem hiding this comment.
As discussed - sounds like a bug. We'll follow up separately.
⯆ @fluid-example/bundle-size-tests: -115 Bytes
Baseline commit: 936f247 |
| this.serializer, | ||
| this.directory.handle, | ||
| ); | ||
| bindHandles(localValue, this.serializer, this.directory.handle); |
There was a problem hiding this comment.
For when we revisit this - I think it can/should go in the isAttached block
With the change to the ordering of getting the attachment summary in #20998 and the testing in #20995 and #21132, we can now remove the workarounds that call bindHandles or makeSerializable when not in an attached state.