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

Remove ComponentContext.documentId, Container.id #2903

Closed
agarwal-navin opened this issue Jul 27, 2020 · 19 comments · Fixed by #7064, #8492, #8680, #8819 or #8840
Closed

Remove ComponentContext.documentId, Container.id #2903

agarwal-navin opened this issue Jul 27, 2020 · 19 comments · Fixed by #7064, #8492, #8680, #8819 or #8840
Assignees
Labels
api area: runtime Runtime related issues epic An issue that is a parent to several other issues
Milestone

Comments

@agarwal-navin
Copy link
Contributor

documentId is a Container level concept and should not be exposed via ComponentContext and ComponentRuntime.
Especially with detachedContainers, Components do not know (or should care) when the Container is attached and the documentId is not available before the Container is attached.

@agarwal-navin agarwal-navin added the area: runtime Runtime related issues label Jul 27, 2020
@ghost ghost added the triage label Jul 27, 2020
@agarwal-navin agarwal-navin self-assigned this Jul 27, 2020
@agarwal-navin agarwal-navin added this to the August 2020 milestone Jul 27, 2020
@curtisman curtisman removed the triage label Aug 10, 2020
@agarwal-navin agarwal-navin modified the milestones: August 2020, Future Aug 17, 2020
@danielroney danielroney added the api label Sep 2, 2020
@danielroney danielroney modified the milestones: Future, Next Sep 2, 2020
@ghost ghost added the status: stale label Jun 8, 2021
@ghost
Copy link

ghost commented Jun 8, 2021

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 ghost closed this as completed Jun 16, 2021
@vladsud
Copy link
Contributor

vladsud commented Jun 29, 2021

Reactivating, as this topic popped up in recent discussions.

@ghost ghost removed the status: stale label Jun 29, 2021
@vladsud vladsud assigned chensixx and unassigned agarwal-navin Jun 29, 2021
@vladsud
Copy link
Contributor

vladsud commented Jun 29, 2021

@chensixx - any chance you can do an assessment here on how it's used and if it's possible to deprecate it?
I'd try to break it into smaller work items:

  1. Remove it from packages/runtime, which means removal from ICotainerContext and client-api
  2. Look at how Container.id is used and if it can be removed over time. Key usages I see (in priority order):
  • Remove it from IContainer & Container public API
  • branch in attributes - I think we can remove it, but need to double check how it's used.
  • getVersion() call - would be great over time to move to using undefined here for same purposes
  • telemetry, using docId - we really need it. I think that's fine to leave as is, so I'd keep it as is, at least for now.

@vladsud vladsud modified the milestones: Next, July 2021 Jun 29, 2021
@vladsud vladsud reopened this Jun 29, 2021
@vladsud vladsud changed the title Stop exposing documentId from ComponentContext and ComponentRuntime Remove ComponentContext.documentId, Container.id Jun 29, 2021
@vladsud
Copy link
Contributor

vladsud commented Jul 27, 2021

Moving to August with assumption that you (@chensixx) are not going to complete it this month. If it's not the case, please move it back :)

@vladsud vladsud modified the milestones: July 2021, August 2021 Jul 27, 2021
@chensixx
Copy link
Contributor

After first attempt of removing document id, I found one instance that's blocking removal: In document.ts, used by flowContainer, used in SharedText. This is also related to issue #2915 so I will fix 2915 first

@chensixx
Copy link
Contributor

@vladsud I have a PR that removes docId from IFluidDataStoreContext and IFluidDataStoreRuntime. is the next step removing docId from everywhere? Since you mentioned remove it from IContainer as well.

@vladsud vladsud modified the milestones: August 2021, September 2021 Aug 24, 2021
@vladsud
Copy link
Contributor

vladsud commented Sep 28, 2021

@chensixx, any chance you can update this item with latest thinking / breakdown / ETA?
I want to have a better handle of the work remaining, and when we think it will be completed.

@markfields
Copy link
Member

markfields commented Dec 31, 2021

Marking as an Epic. This work is staged into many steps and will be long-running.

@markfields markfields added the epic An issue that is a parent to several other issues label Dec 31, 2021
@chensixx
Copy link
Contributor

chensixx commented Jan 7, 2022

the only 2 steps left should be 1. removing container.id and usages, 2. removing containerContext.id after 0.55 is released, then it will be done.
also as Vlad mentioned, we can transition getVersions() to using undefined

@markfields
Copy link
Member

@chensixx Heads up - When you merge that PR this issue will close, but from your last comment it sounds like this issue should live on until the final removal is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment