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

Removed bindToContext from IFluidDataStoreChannel #8269

Merged
merged 2 commits into from
Nov 17, 2021

Conversation

agarwal-navin
Copy link
Contributor

@agarwal-navin agarwal-navin commented Nov 16, 2021

Fixes #7895 and #7927.

IFluidDataStoreChannel::bindToContext was deprecated in 0.50. This has now been removed.
Also, removed bindState tracking from data store runtime since that is now a duplicate of graphAttachState.

@agarwal-navin agarwal-navin requested a review from a team as a code owner November 16, 2021 19:41
@github-actions github-actions bot removed the request for review from a team November 16, 2021 19:41
@github-actions github-actions bot added area: runtime Runtime related issues breaking change This PR or issue would introduce a breaking change public api change Changes to a public API labels Nov 16, 2021
@@ -429,17 +429,17 @@ IFluidDataStoreChannel, IFluidDataStoreRuntime, IFluidHandleContext {
});

this.localChannelContextQueue.clear();
this.bindToContext();
this.updateBindState();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we instead inline the call and get rid of this.bindState? I think (given that's the only entry point) that we ensure it's called only once, so no need to track it? In places where it's tested, can we leverage appropriate this.graphAttachState states?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Removed it. Can you please take a look if the logic looks correct now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me - one less concept to think about!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. Now we just need to rename graphAttachState!

@github-actions github-actions bot requested a review from vladsud November 16, 2021 23:49
@agarwal-navin agarwal-navin merged commit b7510d5 into microsoft:main Nov 17, 2021
agarwal-navin added a commit to agarwal-navin/FluidFramework that referenced this pull request Nov 19, 2021
anthony-murphy added a commit that referenced this pull request Nov 19, 2021
as part of #7613 i'm making a number of type changes that should be backwards and forward compat, but i'd like to have validation for this.

the existing backcompat suppressions are due to #8269. My changes for #7613 will help prevent sprawling breaks due IFluidObject which is part of the issue here
@agarwal-navin agarwal-navin deleted the removeBindToContext branch November 19, 2021 05:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: runtime Runtime related issues breaking change This PR or issue would introduce a breaking change public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove bindToContext from data store interfaces.
3 participants