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

Deprecate unnecessary exports of container-runtime package #23607

Merged
merged 21 commits into from
Jan 23, 2025
Merged

Deprecate unnecessary exports of container-runtime package #23607

merged 21 commits into from
Jan 23, 2025

Conversation

anthony-murphy
Copy link
Contributor

@anthony-murphy anthony-murphy commented Jan 17, 2025 •
edited
Loading

The following types in the @fluidframework/container-runtime are deprecated. These types are unnecessary for external users of this package.

  • currentDocumentVersionSchema
  • DeletedResponseHeaderKey
  • DocumentSchemaValueType
  • DocumentsSchemaController
  • GCFeatureMatrix
  • GCNodeType
  • GCVersion
  • IBlobManagerLoadInfo
  • ICancellableSummarizerController
  • ICancellationToken
  • IConnectableRuntime
  • IContainerRuntimeMetadata
  • ICreateContainerMetadata
  • IDocumentSchema
  • IDocumentSchemaChangeMessage
  • IDocumentSchemaCurrent
  • IDocumentSchemaFeatures
  • IGCMetadata
  • IGCStats
  • IMarkPhaseStats
  • IRefreshSummaryAckOptions
  • ISerializedElection
  • ISubmitSummaryOptions
  • ISummarizerInternalsProvider
  • ISummarizerRuntime
  • ISummaryCancellationToken
  • ISummaryMetadataMessage
  • ISweepPhaseStats
  • Summarizer

related #23644

@github-actions github-actions bot added area: dev experience Improving the experience of devs building on top of fluid 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 Jan 17, 2025
@github-actions github-actions bot removed the area: dev experience Improving the experience of devs building on top of fluid label Jan 17, 2025
@anthony-murphy anthony-murphy force-pushed the test/container-runtime-deprecations branch from 4a5f0d5 to ddd4188 Compare January 17, 2025 23:01
@anthony-murphy anthony-murphy force-pushed the test/container-runtime-deprecations branch from a41535b to dfe2e21 Compare January 17, 2025 23:23
@anthony-murphy
Copy link
Contributor Author

once these deprecations are made internal, we will see about a 50% reduction of exported surface area: https://github.com/microsoft/FluidFramework/compare/test/container-runtime-deprecations-removals

anthony-murphy
anthony-murphy commented Jan 17, 2025
View reviewed changes
packages/runtime/container-runtime/src/channelCollection.ts Outdated Show resolved Hide resolved
packages/runtime/container-runtime/src/channelCollection.ts Outdated Show resolved Hide resolved
packages/runtime/container-runtime/src/channelCollection.ts Outdated Show resolved Hide resolved
packages/runtime/container-runtime/src/channelCollection.ts Outdated Show resolved Hide resolved
packages/runtime/container-runtime/src/channelCollection.ts Outdated Show resolved Hide resolved
packages/runtime/container-runtime/src/summary/summaryManager.ts Outdated Show resolved Hide resolved
packages/runtime/container-runtime/src/summary/summaryManager.ts Outdated Show resolved Hide resolved
packages/runtime/container-runtime/src/summary/summaryManager.ts Outdated Show resolved Hide resolved
packages/runtime/container-runtime/src/summary/summaryManager.ts Outdated Show resolved Hide resolved
packages/runtime/container-runtime/src/summary/summaryManager.ts Outdated Show resolved Hide resolved
@anthony-murphy anthony-murphy force-pushed the test/container-runtime-deprecations branch from b58e4f2 to 611033c Compare January 21, 2025 17:30
@anthony-murphy anthony-murphy marked this pull request as ready for review January 22, 2025 21:07
@anthony-murphy anthony-murphy requested review from a team as code owners January 22, 2025 21:07
alexvy86
alexvy86 approved these changes Jan 22, 2025
View reviewed changes
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.

Approving for docs.

Josmithr
Josmithr reviewed Jan 22, 2025
View reviewed changes
packages/runtime/container-runtime/src/containerRuntime.ts Outdated
@@ -1432,10 +1459,11 @@ export class ContainerRuntime
};

/**
* Summarizer is responsible for coordinating when to send generate and send summaries.
*Summarizer is responsible for coordinating when to send generate and send summaries.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*Summarizer is responsible for coordinating when to send generate and send summaries.
* Summarizer is responsible for coordinating when to send generate and send summaries.

Copy link
Contributor

Choose a reason for hiding this comment

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

There seem to be a number of places where the formatting was (unintentionally?) changed to omit the leading space. We should restore those.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this package was using our recommended eslint config, it would have caught this. Migration to this config is WIP 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. the current linting tools are not great. running autofix tends to create a ton of issue

Josmithr
Josmithr reviewed Jan 22, 2025
View reviewed changes
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 some docs syntax related comments that should be resolved before merging.

@anthony-murphy anthony-murphy force-pushed the test/container-runtime-deprecations branch from b47ff93 to 8be9540 Compare January 22, 2025 22:18
ChumpChief
ChumpChief approved these changes Jan 23, 2025
View reviewed changes
Copy link
Contributor

@ChumpChief ChumpChief left a comment

Choose a reason for hiding this comment

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

Exciting!

packages/runtime/container-runtime/src/containerRuntime.ts Outdated Show resolved Hide resolved
@anthony-murphy anthony-murphy enabled auto-merge (squash) January 23, 2025 17:25
@github-actions GitHub Actions
Copy link
Contributor

🔗 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

markfields
markfields reviewed Jan 23, 2025
View reviewed changes
packages/runtime/container-runtime/src/blobManager/blobManager.ts
@@ -51,6 +51,7 @@ import {
getStorageIds,
summarizeBlobManagerState,
toRedirectTable,
// eslint-disable-next-line import/no-deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Will it help you to find/remove these if you put a keyword on these lines? Or... maybe you can just do git revert on this commit to start the removal branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

npm run lint:fix can remove them

markfields
markfields approved these changes Jan 23, 2025
View reviewed changes
Copy link
Member

@markfields markfields left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Excellent!

FYI I believe @jason-ha had a technique for exporting symbols non-deprecated for @internal which avoids having to do the // eslint-disable-next-line import/no-deprecated everywhere, in case you're interested.

Here it is: https://github.com/microsoft/FluidFramework/pull/23332/files#diff-9155a22383eed097af2d066685a86ffc756942797ddc7d2950c377b5c3bdbd3f

@anthony-murphy anthony-murphy merged commit 3da5b42 into main Jan 23, 2025
38 checks passed
@anthony-murphy anthony-murphy deleted the test/container-runtime-deprecations branch January 23, 2025 18:38
This was referenced Jan 23, 2025
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Josmithr Josmithr Josmithr left review comments

@alexvy86 alexvy86 alexvy86 approved these changes

@ChumpChief ChumpChief ChumpChief approved these changes

@markfields markfields markfields approved these changes

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@anthony-murphy @alexvy86 @ChumpChief @markfields @Josmithr