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

IFluidObject Alternative Without Module Augmentation #7613

Closed
13 tasks done
anthony-murphy opened this issue Sep 28, 2021 · 1 comment · Fixed by #7867
Closed
13 tasks done

IFluidObject Alternative Without Module Augmentation #7613

anthony-murphy opened this issue Sep 28, 2021 · 1 comment · Fixed by #7867
Assignees
Labels
api epic An issue that is a parent to several other issues kr
Milestone

Comments

@anthony-murphy
Copy link
Contributor

anthony-murphy commented Sep 28, 2021

I think we'll need to solve #3264, and #2566 before we can do much here, as the current Module augmentation used by IFluidObject doesn't work correctly with re-exporting, and in general will create a lot of cross version pain. The biggest unknown here is how to deal with types that leverage keyof IFluidObject. I don't think there is a one size fits all approach to replacing that, but i'm exploring the usage patterns to see what we can do.

Originally posted by @anthony-murphy in #6540 (comment)

Proposal:

IFluidObject has two major issues. The first is the module augmentation, which results in unintuitive behavior. For instance it can be necessary to import unused typed to get compilation and intellisence to work correctly. The second is related, in that by augmenting all types onto IFluidObject we end up with an uber type that is easily broken by any change on any interface reachable by IFluidObject, which ends up being most interfaces in the fluid framework: Almost any interface change in the Fluid repo results in IFluidObject typing incompatibility · #2566

The below replacement doesn’t depend on module augmentation, or previous knowledge of the types. Instead it depends on the self-referential provider pattern itself to craft new types for inspection and discovery. The biggest change is that the developer needs to explicitly specify the types they will inspect for. However on code inspection of the repos this seems trivial in all current usage. The one caveat is when the type is just being passed, like in the scope or request pattern, and the caller may inspect it for anything. To solve for this case I’ve add a new type IProvideUnknown. It is just an empty interface, but signals that the type should be inspected with the Provider pattern.

Phase 1 work items

Phase 2 work items

Phase 3 work items - AB#155

Related issues:

@ghost ghost added the triage label Sep 28, 2021
@anthony-murphy anthony-murphy added the epic An issue that is a parent to several other issues label Sep 28, 2021
@anthony-murphy anthony-murphy added this to the October 2021 milestone Sep 28, 2021
@ghost ghost removed the triage label Sep 28, 2021
@anthony-murphy anthony-murphy self-assigned this Sep 28, 2021
@anthony-murphy anthony-murphy added the design-required This issue requires design thought label Sep 28, 2021
@anthony-murphy anthony-murphy changed the title Introduce IFluidObject Alternative IFluidObject Alternative Without Module Augmentation Sep 28, 2021
@nmsimons nmsimons added the kr label Oct 20, 2021
anthony-murphy added a commit that referenced this issue Oct 21, 2021
…inerContext.scope` (#7867)

This change is in preparation for some changes coming to eventually remove IFluidObject as an uber interface, as the coupling of all types off a single interface leads to complex typing issues. This change just moves away from IFluidObject on some types that come from the definitions package, IFluidModule, and IContainerContext.scope. In a follow up i will be removing IFluidObject entirely from these types

related to #7613
@anthony-murphy
Copy link
Contributor Author

@microsoft/fluid-cr-docs

@anthony-murphy anthony-murphy removed the design-required This issue requires design thought label Nov 1, 2021
@curtisman curtisman added the api label Nov 1, 2021
anthony-murphy added a commit that referenced this issue Nov 19, 2021
as part of #7613 i'm making a number of type changes that should be backwards and forward compat, but i'd like to have validation for this.

the existing backcompat suppressions are due to #8269. My changes for #7613 will help prevent sprawling breaks due IFluidObject which is part of the issue here
@nmsimons nmsimons modified the milestones: November 2021, December 2021 Dec 1, 2021
@anthony-murphy anthony-murphy removed this from the February 2022 milestone Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api epic An issue that is a parent to several other issues kr
Projects
None yet
3 participants