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 _createDataStoreWithProps #22996

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Remove _createDataStoreWithProps #22996

wants to merge 9 commits into from

Conversation

tyler-cai-microsoft
Copy link
Contributor

@tyler-cai-microsoft tyler-cai-microsoft commented Nov 6, 2024 •
edited
Loading

AB#856
#22993

Description

Removes APIs that have been deprecated. We have tried to move away from _createDataStoreWithProps as this causes collisions when creating live datastores. Solutions involved aliasing. The original issue with the removal was that there was no way to put in props. Added the initialState parameter to PureDataObjectFactory.createInstanceWithDataStore as pass in props.

Remove APIs

  • IContainerRuntimeBase._createDataStoreWithProps
  • ContainerRuntime._createDataStoreWithProps

Alternatives

  • PureDataObjectFactory.createInstanceWithDataStore allows props to be passed in via the initialState parameter.

Merge Notes

  • Merge only after main is ready for 2.20 changes.

@tyler-cai-microsoft tyler-cai-microsoft requested review from a team as code owners November 6, 2024 00:28
@github-actions github-actions bot added area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc changeset-present public api change Changes to a public API base: main PRs targeted against main branch labels Nov 6, 2024
@tyler-cai-microsoft tyler-cai-microsoft mentioned this pull request Nov 6, 2024
Closed
@tyler-cai-microsoft tyler-cai-microsoft changed the title Legacy breaks/client/2.20/remove create data object with props Remove _createDataStoreWithProps Nov 6, 2024
tylerbutler
tylerbutler approved these changes Nov 6, 2024
View reviewed changes
Copy link
Member

@tylerbutler tylerbutler 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; had some rewording and clarification that would be good to add.

.changeset/gold-maps-cut.md Outdated Show resolved Hide resolved
.changeset/gold-maps-cut.md
Comment on lines +12 to +13
Removed two main APIs, `ContainerRuntime.createDataStoreWithProps` and `IContainerRuntimeBase.createDataStoreWithProps`
has been removed.
Copy link
Member

Choose a reason for hiding this comment

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

The ContainerRuntime.createDataStoreWithProps and IContainerRuntimeBase.createDataStoreWithProps APIs have been removed.

.changeset/gold-maps-cut.md Outdated Show resolved Hide resolved
.changeset/gold-maps-cut.md
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add a note at the bottom with a link to the release notes where these APIs were deprecated? Something like:

The initial deprecations of the now changed or removed types were announced in Fluid Framework v2.4.0: Several MergeTree Client Legacy APIs are now deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have been trying to remove this api for 4 years, but our first deprecation occurred here: #1537

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

msfluid-bot
msfluid-bot reviewed Nov 6, 2024
View reviewed changes
Copy link
Collaborator

@msfluid-bot msfluid-bot 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.

Code Coverage Summary

↑ packages.runtime.container-runtime.src:
Line Coverage Change: No change    Branch Coverage Change: 0.05%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 92.90% 92.95% ↑ 0.05%
Line Coverage 94.50% 94.50% → No change

Baseline commit: 85d0e82
Baseline build: 304645
Happy Coding!!

Code coverage comparison check passed!!

@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Nov 6, 2024 •
edited
Loading

⯆ @fluid-example/bundle-size-tests: -511 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 464.03 KB 463.88 KB ⯆ -154 Bytes
azureClient.js 562.45 KB 562.31 KB ⯆ -140 Bytes
connectionState.js 724 Bytes 724 Bytes ■ No change
containerRuntime.js 261.86 KB 261.69 KB ⯆ -175 Bytes
fluidFramework.js 424.82 KB 424.84 KB ⯅ +14 Bytes
loader.js 134.17 KB 134.19 KB ⯅ +14 Bytes
map.js 42.71 KB 42.71 KB ⯅ +7 Bytes
matrix.js 148.54 KB 148.55 KB ⯅ +7 Bytes
odspClient.js 528.3 KB 528.16 KB ⯆ -140 Bytes
odspDriver.js 97.84 KB 97.86 KB ⯅ +21 Bytes
odspPrefetchSnapshot.js 42.81 KB 42.83 KB ⯅ +14 Bytes
sharedString.js 164.58 KB 164.58 KB ⯅ +7 Bytes
sharedTree.js 415.28 KB 415.29 KB ⯅ +7 Bytes
Total Size 3.36 MB 3.36 MB ⯆ -511 Bytes

Baseline commit: 85d0e82

Generated by 🚫 dangerJS against 2cef339

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 6, 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


> [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...

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:
  447786 links
    3441 destination URLs
       2 URLs ignored
       0 warnings
       1 errors

 ELIFECYCLE  Command failed with exit code 1.

@jason-ha jason-ha linked an issue Dec 11, 2024 that may be closed by this pull request
Remove _createDataStoreWithProps #22993
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msfluid-bot msfluid-bot msfluid-bot left review comments

@tylerbutler tylerbutler tylerbutler approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct 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.

Remove _createDataStoreWithProps
3 participants
@tyler-cai-microsoft @msfluid-bot @tylerbutler