FluidFramework icon indicating copy to clipboard operation
FluidFramework copied to clipboard

Add "reason" to container close APIs

Open markfields opened this issue 3 years ago • 5 comments

This would be helpful in telemetry.

See https://github.com/microsoft/FluidFramework/pull/8619 and https://github.com/microsoft/FluidFramework/pull/6936 for prior thinking here.

Key design point is around compatibility

  • How do we want to stage this in terms of back compat both at runtime between layers and at compile time as partners would take this change.
  • Do we want to switch from multiple function params to a single object? The latter is more conducive to future changes to the signatures here and can enforce consistency across related APIs if applicable.

Side note - while making breaking changes to APIs, let's be sure to update any errors exposed here to properly extend Error, see https://github.com/microsoft/FluidFramework/pull/8527

markfields avatar Dec 29 '21 00:12 markfields

Putting under Epic https://github.com/microsoft/FluidFramework/issues/7999 for now, but there might be a better way to organize this work, stay tuned as part of Jan plan.

markfields avatar Dec 29 '21 00:12 markfields

Goal for Jan is just to get a light design written up, will plan to execute in Feb (due to overfull Jan plan)

markfields avatar Jan 09 '22 04:01 markfields

"This would be helpful in telemetry." -- I need to get a specific use case in here to prioritize. For now moving to March, if it bumps again it'll be to next

markfields avatar Feb 05 '22 00:02 markfields

I had a good conversation with the team about this issue today, discussing the approach in #9334. Main feedback was to step back and make sure the use case and motivation was clear for why we want to change this API and what the right change would be.

There was some discussion (mostly with @skylerjokiel, @curtisman, @anthony-murphy ) of whether different parties would have a different set of args they'd want to pass. The notion of "internal" (i.e. in our repo, but the exact definition is fuzzy) v. "external" (e.g. app host or dataStore code) callers came up. Also a question of why we need reason at all when we have error.

Anyway, I had tinkered with this this morning in case there was a clear step forward to make the breaking change to switch from a single error param to a props object, since we're approaching v1.0 of these packages. But after Helio's talk, I'm less concerned about beating 1.0, and more interested in only making the right change at the right time. 👌

markfields avatar Mar 03 '22 23:03 markfields

This issue has been automatically marked as stale because it has had no activity for 180 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

ghost avatar Sep 15 '22 03:09 ghost