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

Add connect() and disconnect() to IFluidContainer and FluidContainer #9947

Merged
merged 6 commits into from
Apr 20, 2022

Conversation

scottn12
Copy link
Contributor

@scottn12 scottn12 commented Apr 18, 2022

Add connect() and disconnect() to IFluidContainer and FluidContainer. These functions will be made optional on IFluidContainer to avoid a breaking change. In a future release these will be made mandatory functions.

Closes #9745

@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: propertydds area: dev experience Improving the experience of devs building on top of fluid area: examples Changes that focus on our examples 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 breaking change This PR or issue would introduce a breaking change dependencies Pull requests that update a dependency file public api change Changes to a public API base: main PRs targeted against main branch labels Apr 18, 2022
* {@inheritDoc IFluidContainer.connect}
*/
public async connect(): Promise<void> {
return this.container.connect?.();
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 syntax here isn't great. The alternative would be to wait to make these changes once connect()/disconnect() are made mandatory on IContainer. However, I felt it made sense to make the functions mandatory for both IContainer and IFluidContainer in a separate future PR (since they are breaking changes), and then update the syntax here after.

@scottn12 scottn12 requested a review from heliocliu April 18, 2022 17:55
@github-actions github-actions bot removed area: examples Changes that focus on our examples area: dds Issues related to distributed data structures area: dds: propertydds area: dev experience Improving the experience of devs building on top of fluid area: loader Loader related issues dependencies Pull requests that update a dependency file area: runtime Runtime related issues area: tests Tests to add, test infrastructure improvements, etc labels Apr 19, 2022
"broken": {}
"broken": {
"ClassDeclaration_FluidContainer": {
"forwardCompat": false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heliocliu, I believe this situation is similar to the one in my last PR here. Thoughts on if this is okay to stay in main, or if this PR should be in next?

Copy link
Contributor

Choose a reason for hiding this comment

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

this should be fine in main (since i don't think there are any runtime object compatibility considerations here)

@scottn12 scottn12 marked this pull request as ready for review April 19, 2022 17:19
@scottn12 scottn12 requested review from msfluid-bot and a team as code owners April 19, 2022 17:19
@scottn12 scottn12 requested review from markfields and vladsud April 19, 2022 17:23
@scottn12 scottn12 merged commit 17c7b27 into microsoft:main Apr 20, 2022
@scottn12 scottn12 deleted the addConnectDisconnectFluidContainer branch April 20, 2022 17:20
anthony-murphy pushed a commit that referenced this pull request May 18, 2022
…tainer` (#9947)

Add `connect()` and `disconnect()` to `IFluidContainer` and `FluidContainer`. These functions will be made optional on `IFluidContainer` to avoid a breaking change. In a future release these will be made mandatory functions.
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 base: main PRs targeted against main branch breaking change This PR or issue would introduce a breaking change public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add connect() and disconnect() to IFluidContainer
2 participants