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.

Sign up for GitHub

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

Jump to bottom

Remove SummarizerStopReason, ISummarizeEventProps, and ISummarizerEvents #23483

Merged
merged 2 commits into from
Jan 9, 2025
Merged

Remove SummarizerStopReason, ISummarizeEventProps, and ISummarizerEvents #23483

merged 2 commits into from
Jan 9, 2025
+43 −119

Conversation

kian-thompson
Copy link
Contributor

SummarizerStopReason, ISummarizeEventProps, and ISummarizerEvents have all been removed from the "@fluidframework/container-runtime" package and moved to the "@fluidframework/container-runtime-definitions" package.

Context

These types have been moved to the "@fluidframework/container-runtime-definitions" package to be included on the IContainerRuntime interface.

#23217
AB#26524

@kian-thompson kian-thompson requested review from a team as code owners January 7, 2025 19:07
@kian-thompson kian-thompson requested review from pragya91, markfields, jatgarg, tyler-cai-microsoft, rajatch-ff and MarioJGMsoft and removed request for a team January 7, 2025 19:07
@github-actions github-actions bot added base: main PRs targeted against main branch area: runtime Runtime related issues changeset-present public api change Changes to a public API labels Jan 7, 2025
@kian-thompson kian-thompson linked an issue Jan 7, 2025 that may be closed by this pull request
Remove SummarizerStopReason, ISummarizeEventProps, and ISummarizerEvents #23290
Closed
Copilot
Copilot AI reviewed Jan 7, 2025
View reviewed changes

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.

Files not reviewed (2)
  • packages/runtime/container-runtime/package.json: Language not supported
  • packages/runtime/container-runtime/src/index.ts: Evaluated as low risk
jzaffiro
jzaffiro approved these changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@jzaffiro jzaffiro left a comment

Choose a reason for hiding this comment

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

Approved for docs!

markfields
markfields reviewed Jan 8, 2025
View reviewed changes
.changeset/sweet-tires-bathe.md

Removed SummarizerStopReason, ISummarizeEventProps, and ISummarizerEvents

`SummarizerStopReason`, `ISummarizeEventProps`, and `ISummarizerEvents` have all been removed from the `"@fluidframework/container-runtime"` package. Please migrate all uses of these APIs to their respective copies in the `"@fluidframework/container-runtime-definitions"` package.
Copy link
Member

Choose a reason for hiding this comment

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

@jason-ha was suggesting we link to the release note where the deprecation was announced, rather than having to restate everything again in duplicate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep - but okay if the migration notes are short like this.

markfields
markfields reviewed Jan 8, 2025
View reviewed changes
packages/runtime/container-runtime/src/test/types/validateContainerRuntimePrevious.generated.ts
@@ -742,6 +742,7 @@ declare type current_as_old_for_Interface_ISubmitSummaryOptions = requireAssigna
* typeValidation.broken:
* "Interface_ISummarizeEventProps": {"forwardCompat": false}
*/
// @ts-expect-error compatibility expected to be broken
Copy link
Member

Choose a reason for hiding this comment

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

@CraigMacomber -- I thought when something was removed, it got a special new entry in this file. Did that change?

Copy link
Contributor

Choose a reason for hiding this comment

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

The type tests don't understand "removal". The tests are generated purely from the prior package API and the break settings of the current package.
The testing shouldn't care if the API still exists or not - just that the new API surface is (isn't) the same as the old.
(This was a nice change in strategy that Craig put in place.)

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 9, 2025

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170508 links
    1603 destination URLs
    1842 URLs ignored
       0 warnings
       0 errors

@kian-thompson kian-thompson merged commit 6666d49 into microsoft:main Jan 9, 2025
32 checks passed
@kian-thompson kian-thompson deleted the 26524-remove-deprecated-summmarizer-types branch January 9, 2025 02:16
@github-actions github-actions bot mentioned this pull request Jan 9, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot Copilot left review comments

@jzaffiro jzaffiro jzaffiro approved these changes

@jatgarg jatgarg jatgarg approved these changes

@jason-ha jason-ha jason-ha approved these changes

@markfields markfields markfields approved these changes

@pragya91 pragya91 Awaiting requested review from pragya91 pragya91 was automatically assigned from microsoft/team-rajat

@tyler-cai-microsoft tyler-cai-microsoft Awaiting requested review from tyler-cai-microsoft tyler-cai-microsoft was automatically assigned from microsoft/team-rajat

@rajatch-ff rajatch-ff Awaiting requested review from rajatch-ff rajatch-ff was automatically assigned from microsoft/team-rajat

@MarioJGMsoft MarioJGMsoft Awaiting requested review from MarioJGMsoft MarioJGMsoft was automatically assigned from microsoft/team-rajat

Assignees
No one assigned
Labels
area: runtime Runtime related issues base: main PRs targeted against main branch changeset-present public api change Changes to a public API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove SummarizerStopReason, ISummarizeEventProps, and ISummarizerEvents
5 participants
@kian-thompson @markfields @jason-ha @jatgarg @jzaffiro