Remove back-compat code related to the op reentry checks #22842

MarioJGMsoft · 2024-10-17T22:23:25Z

Description

In light of Remove ability to reject reentrant ops (#20621) · microsoft/FluidFramework@c9d1562 (github.com), updating this to be about finishing the removal of ensureNoDataModelChanges.

The following uses of ensureNoDataModelChanges will be removed:

packages\runtime\container-runtime\src\channelCollection.ts:
- ensureNoDataModelChanges: (...args) => {
packages\runtime\container-runtime\src\dataStoreContext.ts:
- public ensureNoDataModelChanges(callback: () => T): T {
packages\runtime\runtime-definitions\src\dataStoreContext.ts:
- ensureNoDataModelChanges(callback: () => T): T;
packages\runtime\test-runtime-utils\src\mocksDataStoreContext.ts:
- public ensureNoDataModelChanges(callback: () => T): T {

msfluid-bot

Code Coverage Summary

↑ packages.runtime.container-runtime.src:
Line Coverage Change: 0.02% Branch Coverage Change: No change

Metric Name	Baseline coverage	PR coverage	Coverage Diff
Branch Coverage	92.90%	92.90%	→ No change
Line Coverage	94.53%	94.55%	↑ 0.02%

Baseline commit: 952e805
Baseline build: 306581
Happy Coding!!

Code coverage comparison check passed!!

msfluid-bot · 2024-10-17T23:44:51Z

⯆ @fluid-example/bundle-size-tests: -347 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	465.86 KB	465.75 KB	⯆ -113 Bytes
azureClient.js	563.18 KB	563.09 KB	⯆ -99 Bytes
connectionState.js	724 Bytes	724 Bytes	■ No change
containerRuntime.js	262.48 KB	262.35 KB	⯆ -134 Bytes
fluidFramework.js	426.99 KB	427.01 KB	⯅ +14 Bytes
loader.js	134.18 KB	134.19 KB	⯅ +14 Bytes
map.js	42.71 KB	42.71 KB	⯅ +7 Bytes
matrix.js	149.84 KB	149.85 KB	⯅ +7 Bytes
odspClient.js	528.97 KB	528.88 KB	⯆ -99 Bytes
odspDriver.js	97.88 KB	97.9 KB	⯅ +21 Bytes
odspPrefetchSnapshot.js	42.81 KB	42.83 KB	⯅ +14 Bytes
sharedString.js	165.77 KB	165.78 KB	⯅ +7 Bytes
sharedTree.js	417.45 KB	417.46 KB	⯅ +7 Bytes
Total Size	3.37 MB	3.37 MB	⯆ -347 Bytes

Baseline commit: 952e805

Generated by 🚫 dangerJS against 5a45ae4

jason-ha · 2024-10-18T18:59:00Z

packages/runtime/test-runtime-utils/api-report/test-runtime-utils.legacy.alpha.api.md

    // (undocumented)
-    ensureNoDataModelChanges<T>(callback: () => T): T;


This technically should have been marked as deprecated before now. To be completely strict we would not remove this from the mock until properly deprecated. This is test-runtime-utils and maybe there is a different contract there. I will let API CR make final call.

Unless there is a reason we have to remove this in this PR and we are confident that none of our partners are using it (beyond just Loop), then I think it would be best to stick to the process and retain this as deprecated for future removal.

I see that we had an internal-facing comment noting it would be removed in 2.0, but our customers would have had no awareness of this, so I think it's worth erring on the side of caution.

So, it seems like "ensureNoDataModelChanges" was deprecated on the interface and now he is removing it from the interface and classes.
Is it wrong to do so? In order to remove it, should it be marked as deprecated on interfaces and all classes implementing that interface?

@jason-ha and @Josmithr Please take a look at this comment when you get time. We need to complete this breaking change PR by next week. Thanks.

Sorry, missed the earlier responses. Yes, strictly speaking any implementation of the interface that declares the same properties should also mark the members as deprecated. There is nothing stopping MockFluidDataStoreContext from continuing to expose a ensureNoDataModelChanges member even if the interface it extends plans to remove it from the interface. So, to be explicit, any implementations for which the member is deprecated should be marked so as well.

Unless there is an urgent need to remove this, it would probably be better to mark the appropriate implementation members as @deprecated and add them to the issue tracking @legacy breaks we plan to make in 2.20.

I don't think there is anything stopping us from removing the deprecated interface member, though.

I would be okay making an exception for this one since the expected use is for mock to be used in places that would use ensureNoDataModelChanges via interface. Technically, we should have deprecated before and should wait, but I would be surprised if there is fallout.

@Josmithr, are you also okay with this exception? Latest version has this removed per @jatgarg's suggestion to go the exception route.

@MarioJGMsoft, please make sure you have tested a build with these changes using the FF+Loop integration process. Reply with the build results link. Thanks!
Reference docs: https://eng.ms/docs/experiences-devices/opg/office-shared/fluid-framework/fluid-framework-internal/fluid-framework/docs/dev/monitoring/ff-loop-testing

@Josmithr, are you also okay with this exception? Latest version has this removed per @jatgarg's suggestion to go the exception route.

I don't have a strong opinion either way. Introducing a break to something with "Mock" in the name seems safer than most alternatives, so I don't see any harm in it. But I do think we should generally try to be consistent with this stuff. 🤷‍♂️

packages/runtime/test-runtime-utils/src/mocksDataStoreContext.ts

packages/runtime/container-runtime/src/dataStoreContext.ts

packages/runtime/container-runtime/src/channelCollection.ts

jason-ha

Be sure to add a changeset for the removals and if we go with deprecation for the mock, then a deprecation note for it.

packages/runtime/test-runtime-utils/src/mocksDataStoreContext.ts

jason-ha · 2024-11-12T00:26:16Z

packages/runtime/test-runtime-utils/api-report/test-runtime-utils.legacy.alpha.api.md

    // (undocumented)
-    ensureNoDataModelChanges<T>(callback: () => T): T;


I would be okay making an exception for this one since the expected use is for mock to be used in places that would use ensureNoDataModelChanges via interface. Technically, we should have deprecated before and should wait, but I would be surprised if there is fallout.

packages/runtime/container-runtime/src/channelCollection.ts

packages/runtime/test-runtime-utils/src/mocksDataStoreContext.ts

.changeset/icy-webs-help.md

jason-ha · 2024-11-13T18:20:44Z

I think PR description should be updated. It seems to reference past things rather than describe the current change. Probably should be similar to the changeset content.

jatgarg · 2024-11-13T19:28:12Z

@MarioJGMsoft Since @jason-ha suggested we can remove the test runtime utils one too in this PR, lets remove that as well. Also update the changeset accordingly.

.changeset/icy-webs-help.md

github-actions · 2024-11-13T22:49:57Z

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:linkcheck /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test ci:start 1313 linkcheck:full

1: starting server using command "npm run ci:start"
and when url "[ 'http://127.0.0.1:1313' ]" is responding with HTTP status code 200
running tests using command "npm run linkcheck:full"


> [email protected] ci:start
> http-server ./public --port 1313 --silent


> [email protected] linkcheck:full
> npm run linkcheck:fast -- --external


> [email protected] linkcheck:fast
> linkcheck http://localhost:1313 --skip-file skipped-urls.txt --external

Crawling...

Stats:
  443760 links
    3414 destination URLs
       2 URLs ignored
       0 warnings
       0 errors

Josmithr

Docs changes look good! Thanks!

Removing ensureNoDataModelChanges

8e256ba

github-actions bot added area: runtime Runtime related issues base: main PRs targeted against main branch labels Oct 17, 2024

Fixed back-compat breaks

d036753

github-actions bot added area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: tests Tests to add, test infrastructure improvements, etc labels Oct 17, 2024

updated api runtime

c3698df

MarioJGMsoft requested a review from a team as a code owner October 17, 2024 22:52

github-actions bot added the public api change Changes to a public API label Oct 17, 2024

MarioJGMsoft requested review from a team, pragya91, markfields, jatgarg, tyler-cai-microsoft, kian-thompson and rajatch-ff and removed request for a team October 17, 2024 22:54

MarioJGMsoft linked an issue Oct 17, 2024 that may be closed by this pull request

Remove back-compat code related to the op reentry checks #22844

Closed

msfluid-bot reviewed Oct 17, 2024

View reviewed changes

jason-ha reviewed Oct 18, 2024

View reviewed changes

MarioJGMsoft mentioned this pull request Oct 18, 2024

Remove back-compat code related to the op reentry checks #22844

Closed

jatgarg requested review from Josmithr and jason-ha November 7, 2024 17:47

MarioJGMsoft added 3 commits November 11, 2024 23:48

marked as deprecated based on comments

4de92ce

fixed merge

817aefa

Merge branch 'main' into ensureNoDataModelChangers

6e52198

github-actions bot removed area: tests Tests to add, test infrastructure improvements, etc area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct labels Nov 12, 2024

jatgarg reviewed Nov 12, 2024

View reviewed changes

packages/runtime/test-runtime-utils/src/mocksDataStoreContext.ts Outdated Show resolved Hide resolved

jatgarg reviewed Nov 12, 2024

View reviewed changes

packages/runtime/container-runtime/src/dataStoreContext.ts Show resolved Hide resolved

jatgarg reviewed Nov 12, 2024

View reviewed changes

packages/runtime/container-runtime/src/channelCollection.ts Show resolved Hide resolved

jason-ha reviewed Nov 12, 2024

View reviewed changes

Added changeset and updated code comments

310f952

MarioJGMsoft requested a review from a team as a code owner November 12, 2024 18:11

github-actions bot added the changeset-present label Nov 12, 2024

jason-ha approved these changes Nov 12, 2024

View reviewed changes

packages/runtime/test-runtime-utils/src/mocksDataStoreContext.ts Outdated Show resolved Hide resolved

.changeset/icy-webs-help.md Outdated Show resolved Hide resolved

jason-ha reviewed Nov 12, 2024

View reviewed changes

.changeset/icy-webs-help.md Outdated Show resolved Hide resolved

Applied changes based on comments

352b303

jason-ha approved these changes Nov 13, 2024

View reviewed changes

MarioJGMsoft added 2 commits November 13, 2024 19:35

Applied changes based on comments

bffeadd

Merge branch 'main' into ensureNoDataModelChangers

c22885b

jatgarg approved these changes Nov 13, 2024

View reviewed changes

Josmithr reviewed Nov 13, 2024

View reviewed changes

.changeset/icy-webs-help.md Outdated Show resolved Hide resolved

Josmithr reviewed Nov 13, 2024

View reviewed changes

.changeset/icy-webs-help.md Outdated Show resolved Hide resolved

MarioJGMsoft added 2 commits November 13, 2024 22:37

Applied changes based on comments

1598800

Applied changes based on comments

5a45ae4

MarioJGMsoft mentioned this pull request Nov 13, 2024

Test Remove back-compat code #23082

Closed

Josmithr approved these changes Nov 14, 2024

View reviewed changes

MarioJGMsoft merged commit 3aff19a into microsoft:main Nov 14, 2024
32 checks passed

github-actions bot mentioned this pull request Nov 14, 2024

Upcoming Release: FluidFramework v2.21.0 #22101

Open

MarioJGMsoft deleted the ensureNoDataModelChangers branch November 14, 2024 18:04

github-actions bot mentioned this pull request Nov 14, 2024

Upcoming Release: FluidFramework v2.21.0 tylerbutler/FluidFramework#26

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove back-compat code related to the op reentry checks #22842

Remove back-compat code related to the op reentry checks #22842

MarioJGMsoft commented Oct 17, 2024 •

edited

Loading

msfluid-bot left a comment •

edited

Loading

msfluid-bot commented Oct 17, 2024 •

edited

Loading

jason-ha Oct 18, 2024

Josmithr Oct 18, 2024 •

edited

Loading

jatgarg Nov 6, 2024

jatgarg Nov 7, 2024

Josmithr Nov 11, 2024

jason-ha Nov 12, 2024

jason-ha Nov 13, 2024

jason-ha Nov 13, 2024

Josmithr Nov 13, 2024

jason-ha left a comment

jason-ha Nov 12, 2024

jason-ha commented Nov 13, 2024

jatgarg commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

Josmithr left a comment

		// (undocumented)
		ensureNoDataModelChanges<T>(callback: () => T): T;

Remove back-compat code related to the op reentry checks #22842

Remove back-compat code related to the op reentry checks #22842

Conversation

MarioJGMsoft commented Oct 17, 2024 • edited Loading

Description

msfluid-bot left a comment • edited Loading

Choose a reason for hiding this comment

Code Coverage Summary

Code coverage comparison check passed!!

msfluid-bot commented Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Josmithr Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jason-ha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jason-ha commented Nov 13, 2024

jatgarg commented Nov 13, 2024

github-actions bot commented Nov 13, 2024

linkcheck output

Josmithr left a comment

Choose a reason for hiding this comment

MarioJGMsoft commented Oct 17, 2024 •

edited

Loading

msfluid-bot left a comment •

edited

Loading

msfluid-bot commented Oct 17, 2024 •

edited

Loading

Josmithr Oct 18, 2024 •

edited

Loading