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

Remove deprecated inbound and outbound queues on IDeltaManager #22282

Merged
merged 20 commits into from
Nov 7, 2024

Conversation

jatgarg
Copy link
Contributor

@jatgarg jatgarg commented Aug 21, 2024

Description

AB#7202

1.) Remove deprecated inbound and outbound queues on IDeltaManager.
2.) Move them to IDeltaManagerFull so that internal Fluid layers can still use it but not the apps.

@jatgarg jatgarg self-assigned this Aug 21, 2024
@jatgarg jatgarg requested review from a team as code owners August 21, 2024 04:46
@jatgarg jatgarg marked this pull request as draft August 21, 2024 04:46
@github-actions github-actions bot added area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: loader Loader related issues area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc public api change Changes to a public API base: main PRs targeted against main branch labels Aug 21, 2024
@jatgarg jatgarg requested review from a team, pragya91, markfields, tyler-cai-microsoft, kian-thompson and rajatch-ff and removed request for a team August 21, 2024 04:47
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Aug 21, 2024

@fluid-example/bundle-size-tests: +890 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 463.61 KB 463.77 KB +171 Bytes
azureClient.js 562.45 KB 562.71 KB +276 Bytes
connectionState.js 724 Bytes 724 Bytes No change
containerRuntime.js 261.86 KB 262 KB +150 Bytes
fluidFramework.js 424.82 KB 424.84 KB +14 Bytes
loader.js 134.17 KB 134.19 KB +19 Bytes
map.js 42.71 KB 42.71 KB +7 Bytes
matrix.js 148.32 KB 148.33 KB +7 Bytes
odspClient.js 528.29 KB 528.48 KB +190 Bytes
odspDriver.js 97.84 KB 97.86 KB +21 Bytes
odspPrefetchSnapshot.js 42.81 KB 42.83 KB +14 Bytes
sharedString.js 164.17 KB 164.17 KB +7 Bytes
sharedTree.js 415.28 KB 415.29 KB +7 Bytes
Total Size 3.36 MB 3.36 MB +890 Bytes

Baseline commit: 8d47008

Generated by 🚫 dangerJS against 6710b81

Copy link
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

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

Didn't go through the whole thing, just a couple of comments below, and a general comment that I thought we were still holding off on any breaking changes (including to @alpha/@legacy APIs).

packages/common/container-definitions/src/deltas.ts Outdated Show resolved Hide resolved
packages/loader/container-loader/src/loadPaused.ts Outdated Show resolved Hide resolved
@jatgarg
Copy link
Contributor Author

jatgarg commented Aug 21, 2024

Didn't go through the whole thing, just a couple of comments below, and a general comment that I thought we were still holding off on any breaking changes (including to @alpha/@legacy APIs).

Yeah, I am not going to merge it until it is allowed to make breaking changes, but want to get review/approval so that I can just merge when it is allowed and to know if it is ok to make this change.

@jatgarg jatgarg requested review from alexvy86 and vladsud August 21, 2024 22:30
@jatgarg jatgarg requested review from Josmithr and jason-ha November 4, 2024 23:39
@jatgarg
Copy link
Contributor Author

jatgarg commented Nov 5, 2024

@jason-ha and @Josmithr Please take a look at this PR when you get time. Thanks.

Copy link
Contributor

@jason-ha jason-ha left a comment

Choose a reason for hiding this comment

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

Be sure to update PR description - still says IDeltaManagerInternal

packages/test/test-utils/src/containerUtils.ts Outdated Show resolved Hide resolved
packages/test/test-utils/src/containerUtils.ts Outdated Show resolved Hide resolved
@jason-ha
Copy link
Contributor

jason-ha commented Nov 7, 2024

Two new notes:

  1. We just had another break change break Loop. I would recommend running (rerunning) the Loop integration tests as due diligence.
  2. Please add a change set. Tyler just added a "Legacy API Changes" section so the changeset can target that. (Set "section" to legacy)

@jatgarg jatgarg requested a review from a team as a code owner November 7, 2024 00:52
Copy link
Contributor

@anthony-murphy anthony-murphy left a comment

Choose a reason for hiding this comment

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

api changes look good

@jatgarg
Copy link
Contributor Author

jatgarg commented Nov 7, 2024

Integration pipeline with Loop succeeded for the breaking change: https://dev.azure.com/office/OC/_build/results?buildId=32410848&view=results

Comment on lines +143 to +147
"broken": {
"Interface_IFluidContainerInternal": {
"backCompat": false
}
},
Copy link
Member

Choose a reason for hiding this comment

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

I'm very naive - is this safe? cc: @ChumpChief

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The breaking changes will have these. However, since we first deprecated it and now properly removing it, then it is fine.

jatgarg and others added 2 commits November 7, 2024 10:40
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
Copy link
Contributor

github-actions bot commented Nov 7, 2024

🔗 Found some broken links! 💔

Run a link check locally to find them. See
https://github.com/microsoft/FluidFramework/wiki/Checking-for-broken-links-in-the-documentation for more information.

linkcheck output


> fluidframework-docs@0.25.0 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"


> fluidframework-docs@0.25.0 ci:start
> http-server ./public --port 1313 --silent


> fluidframework-docs@0.25.0 linkcheck:full
> npm run linkcheck:fast -- --external


> fluidframework-docs@0.25.0 linkcheck:fast
> linkcheck http://localhost:1313 --skip-file skipped-urls.txt --external

Crawling...

http://localhost:1313/docs/start/tree-start/
- (3430:89) 'here' => https://github.com/microsoft/FluidFramework/blob/main/packages/dds/tree/docs/main/merge-semantics.md (HTTP 404)


Stats:
  443676 links
    3413 destination URLs
       2 URLs ignored
       0 warnings
       1 errors

 ELIFECYCLE  Command failed with exit code 1.

@jatgarg jatgarg enabled auto-merge (squash) November 7, 2024 19:03

The inbound and outbound properties have been removed from IDeltaManager

The inbound and outbound properties were deprecated in a previous release and have been removed from IDeltaManager. Please check pull request [#19636](https://github.com/microsoft/FluidFramework/pull/19636) for alternative APIs.
Copy link
Contributor

@Josmithr Josmithr Nov 7, 2024

Choose a reason for hiding this comment

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

I would recommend including the migration / replacement API instructions directly here, rather than linking to a PR / release notes. That's a lot of navigation steps for our users to jump through to get the information they need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will paste here. I added link to the release notes as well. But I can certainly copy those here.

Copy link
Contributor

@Josmithr Josmithr left a comment

Choose a reason for hiding this comment

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

Left a small changeset recommendation, but otherwise docs changes look good.

@jatgarg jatgarg merged commit 45a5769 into microsoft:main Nov 7, 2024
44 checks passed
@jatgarg
Copy link
Contributor Author

jatgarg commented Nov 7, 2024

Will make the change in another PR. Auto merge was on, so it got merged.

jatgarg added a commit that referenced this pull request Nov 7, 2024
…anager (#23024)

## Description

Add chngeset for removing the inbound and outbound queues on
iDeltamanager. Refer to this PR for extra details:
#22282

---------

Co-authored-by: Jatin Garg <jatingarg@Jatins-MacBook-Pro-2.local>
Co-authored-by: Tyler Butler <tyler@tylerbutler.com>
@jatgarg jatgarg deleted the queue2 branch November 7, 2024 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: loader Loader related issues area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch changeset-present public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecated IDeltaManager.inbound and IDeltaManager.outbound removal
9 participants