Almost any interface change in the Fluid repo results in IFluidObject typing incompatibility #2566

leeviana · 2020-06-16T20:43:38Z

Describe the bug

Anything that references an IFluidObject type from different versions are pretty much always guaranteed to be typescript incompatible in a "major version" sort of way because of the number of "provides" contracts on IFluidObject . This means we can pretty much never have two different versions of packages on either side of an IFluidObject exchange, regardless of whether you're even using any interfaces that have actually changed. For instance, if somebody makes a random change to IFluidDataStoreContext, through a really deep dependency chain involving views and IHostRuntime, this essentially means a major bump in IFluidObject/core-interfaces.

To Reproduce

Make a change to almost any interface in the Fluid repo. Likely the IFluidObject interface in the version of the code published is not compatible with the version of code right before.

Expected behavior

IFluidObject interfaces should be somewhat durable across Framework changes, especially in interfaces that aren't being immediately used.

leeviana · 2020-06-16T20:44:01Z

@anthony-murphy

markfields · 2020-08-18T21:48:22Z

We chatted about this issue today, and I wanted to jot down some notes.

We discussed the approach of removing IFluidObject from API signatures, and instead using it as-needed to cast through to get a type safe interface implementation. This will of course fix the TypeScript compilation errors, but we're still left with the runtime impact of interface changes across versions, so not much appears to be gained here actually.

The bulletproof way to deal with this is to never change any interface. Instead of modifying IFoo, you create IFoo2 with the changes, leaving IFoo intact. This way, within the Fluid ecosystem, if you ever find yourself with an object with a property IFoo you know its shape definitively.

But this is pretty onerous, so we're hopeful to find a more elegant solution.

markfields · 2020-08-19T21:13:46Z

Update to my previous comment - even if we neutralize the type in API signatures, there's still a compile-time trap when a particular bit of code (e.g. a Container specification) imports two different module (e.g. 2 data stores) built/published against different versions of the runtime, with different definitions of IFluidObject.

I wonder if we can't just put a stake in the ground to say that when authoring a Container all included Data Stores and DDS's must be targeting the same version of the runtime? Is this just kicking the can until we get to dynamic loading? Does it even solve this issue?

markfields · 2020-08-19T21:21:33Z

Also, I think this issue would benefit from a concrete repro case and description of what specific errors come up. @leeviana if you can point to a PR or something that fixed this issue before, or any other collateral that would be great. On our side, @anthony-murphy and I discussed just crafting a toy example where some code imports two different modules, each with their own differing definitions of IFluidObject. Then we can not only see the errors but play around with different scenarios/tactics.

Speaking of tactics, a few we've discussed:

Audit interfaces being registered with IFluidObject to shed some that may be abusing the pattern. @anthony-murphy and I did a quick pass and I opened issue Remove certain "internal" interfaces from IFluidObject #3264 for some potential low-hanging fruit here. This would hopefully reduce the churn.
See if JavaScript's duck typing lets us be a bit loosey-goosey (all the water fowl!) in making changes to interfaces - e.g...
- An extreme example would be to make everything on our interfaces optional via Partial<T>, and then even once you have an interface you need to query to see which members are actually there. I think this has a decent shot at working with the double import.
- Using a discriminated union of versions of an interface, so the thing you get back only has the version field and anything common across all versions. Not sure how the double import would handle this... probably won't work.
Incorporate some kind of "strong name" semantics where interfaces have the friendly name concatenated with a guid, so you can use the guid to definitively address a particular interface, but you still get friendly names. We've discussed a similar scheme for naming factories. And just don't ever change the interface without renaming it / regenerating a guid.

markfields · 2020-08-19T21:24:14Z

@curtisman, @anthony-murphy - These notes all came from discussions we had, anything else to add or clarify?

leeviana · 2020-08-19T22:55:49Z

I don't think we have a specific PR because this particular issues prevents us from checking in any code :) I believe I created this issue during one of Kenneth's particularly convoluted FFX bumps where I suggested that in order to reduce regressions/catch them more quickly, we check in the component/container version bump first and then the host version updates second. However, all the contracts around IComponent were broken/unable to be reused around really random things ten layers deep into each interface and ending in something very much unrelated like an interface change in containerRuntime etc.

For example, scriptor is both a host and a component. So its component dependencies needed to be updated. However it could not use the IComponent output of the Loader.resolve() because the Loader's version of IComponent was incompatible with the component's version of IComponent because of aforementioned really random changes that had nothing to do with IComponent interfaces that were used.

leeviana · 2020-08-19T22:58:20Z

You can probably repro this issue quite easily in the bohemia repo by picking any version bump and updating some dependencies in components/containers and not host/Loader code and seeing how it fails.

markfields · 2020-08-20T01:12:54Z

Ah of course, that makes sense. Thanks for spelling it out for me :)

DLehenbauer · 2020-09-22T20:21:38Z

Found an example of 'IFluidDataStoreFactory' being incompatible with 'IFluidDataStoreFactory', where both packages are pulling v0.26.1 from npmjs:

ERROR in /workspaces/ps/danlehen/graphql/client/src/containerCode.ts
[tsl] ERROR in /workspaces/ps/danlehen/graphql/client/src/containerCode.ts(21,5)
      TS2345: Argument of type 'Map<string, Promise<Readonly<Partial<IProvideFluidDataStoreRegistry & IProvideFluidDataStoreFactory>>>>' is not assignable to parameter of type 'NamedFluidDataStoreRegistryEntries'.
  The types returned by '[Symbol.iterator]().next(...)' are incompatible between these types.
    Type 'IteratorResult<[string, Promise<Readonly<Partial<IProvideFluidDataStoreRegistry & IProvideFluidDataStoreFactory>>>], any>' is not assignable to type 'IteratorResult<NamedFluidDataStoreRegistryEntry, any>'.
      Type 'IteratorYieldResult<[string, Promise<Readonly<Partial<IProvideFluidDataStoreRegistry & IProvideFluidDataStoreFactory>>>]>' is not assignable to type 'IteratorResult<NamedFluidDataStoreRegistryEntry, any>'.
        Type 'IteratorYieldResult<[string, Promise<Readonly<Partial<IProvideFluidDataStoreRegistry & IProvideFluidDataStoreFactory>>>]>' is not assignable to type 'IteratorYieldResult<NamedFluidDataStoreRegistryEntry>'.
          Type '[string, Promise<Readonly<Partial<IProvideFluidDataStoreRegistry & IProvideFluidDataStoreFactory>>>]' is not assignable to type 'NamedFluidDataStoreRegistryEntry'.
            Type 'Promise<Readonly<Partial<import("/workspaces/ps/danlehen/graphql/common/node_modules/@fluidframework/runtime-definitions/dist/dataStoreRegistry").IProvideFluidDataStoreRegistry & import("/workspaces/ps/danlehen/graphql/common/node_modules/@fluidframework/runtime-definitions/dist/dataStoreFactory").IProvideFluidDataS...' is not assignable to type 'Promise<Readonly<Partial<import("/workspaces/ps/danlehen/graphql/client/node_modules/@fluidframework/runtime-definitions/dist/dataStoreRegistry").IProvideFluidDataStoreRegistry & import("/workspaces/ps/danlehen/graphql/client/node_modules/@fluidframework/runtime-definitions/dist/dataStoreFactory").IProvideFluidDataS...'.
              Type 'Readonly<Partial<import("/workspaces/ps/danlehen/graphql/common/node_modules/@fluidframework/runtime-definitions/dist/dataStoreRegistry").IProvideFluidDataStoreRegistry & import("/workspaces/ps/danlehen/graphql/common/node_modules/@fluidframework/runtime-definitions/dist/dataStoreFactory").IProvideFluidDataStoreFact...' is not assignable to type 'Readonly<Partial<import("/workspaces/ps/danlehen/graphql/client/node_modules/@fluidframework/runtime-definitions/dist/dataStoreRegistry").IProvideFluidDataStoreRegistry & import("/workspaces/ps/danlehen/graphql/client/node_modules/@fluidframework/runtime-definitions/dist/dataStoreFactory").IProvideFluidDataStoreFact...'.
                Types of property 'IFluidDataStoreFactory' are incompatible.
                  Type 'import("/workspaces/ps/danlehen/graphql/common/node_modules/@fluidframework/runtime-definitions/dist/dataStoreFactory").IFluidDataStoreFactory | undefined' is not assignable to type 'import("/workspaces/ps/danlehen/graphql/client/node_modules/@fluidframework/runtime-definitions/dist/dataStoreFactory").IFluidDataStoreFactory | undefined'.
                    Type 'import("/workspaces/ps/danlehen/graphql/common/node_modules/@fluidframework/runtime-definitions/dist/dataStoreFactory").IFluidDataStoreFactory' is not assignable to type 'import("/workspaces/ps/danlehen/graphql/client/node_modules/@fluidframework/runtime-definitions/dist/dataStoreFactory").IFluidDataStoreFactory'.

Fixed by casting to 'any' here:

export const GraphContainerRuntimeFactory = new ContainerRuntimeFactoryWithDefaultDataStore(
    GraphInstantiationFactory.type,
    new Map([
        GraphInstantiationFactory.registryEntry as any,   // <-- Fixed by 'as any'
    ]),
);

curtisman · 2020-09-22T21:02:59Z

@DLehenbauer what are the versions of @fluidframework/runtime-definitions in /workspaces/ps/danlehen/graphql/common vs /workspaces/ps/danlehen/graphql/client, The interface shouldn't change between 0.26.x release.

leeviana · 2020-09-22T21:03:54Z

Yeah, unfortunately things like this have led to a lot of similar 'any' casting where we test different versions of host/loader etc. :(

DLehenbauer · 2020-09-22T21:36:48Z

@curtisman - It's '0.26.1' in both cases. I did a recursive diff of all the fluidframework packages like this:

diff -r /workspaces/ps/danlehen/graphql/common/node_modules/@fluidframework /workspaces/ps/danlehen/graphql/client/node_modules/@fluidframework

...and they are 100% identical (except for the "_where:" field generated by 'npm i' in package.json... which differs as expected).

Seems to be WebPack related ('client' compiles successfully w/tsc.)

Remove the unused interfaces on FluidObject ISharedString ISharedObject IChannel In addition to being unused these interfaces represent bad practices. A DataStore shouldn't externally expose it's dds for others to access directly. It should instead expose and API and control the data model of the dds. While gernerally unused there is a coupling between our scribe tool and our shared text examples that leverages the ISharedString interface. To remove this interface from IFluidObject, which broadly exposes this bad practice, I've move it to using the request pattern. While still not ideal, this localizes the contract between the two rather than leaking this across the system. Related #3264 Related #2566

ghost · 2021-06-08T00:02:47Z

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!

leeviana · 2021-06-08T00:46:01Z

This is still an issue, what is the standard here, just to leave a comment to prevent if from being closed?

markfields · 2021-08-02T22:21:09Z

Still interested in this but about to go on leave so removing my assignment.

Stevenic · 2021-10-20T16:16:26Z

FYI... We ran into a similar issue with the Bot Framework SDK's. The lesson we learned is that you should always try to favor classes over interfaces as interfaces are essentially immutable and can almost never be updated. The advantage of classes is that you can easily extend them in future releases and provide default behaviors so that you don't break compatibility with previous versions.

anthony-murphy · 2021-10-20T16:39:02Z

@Stevenic Thanks for this! curious if you had cases where consumer would be the implementor? did you end up going with abstract classes is these cases?

anthony-murphy · 2021-11-01T17:27:16Z

will be fixed by #7613

leeviana added the bug Something isn't working label Jun 16, 2020

ghost added the triage label Jun 16, 2020

curtisman assigned anthony-murphy Jul 29, 2020

curtisman added this to the August 2020 milestone Aug 10, 2020

curtisman removed the triage label Aug 10, 2020

anthony-murphy changed the title ~~Almost any interface change in the Fluid repo results in IComponent typing incompatibility~~ Almost any interface change in the Fluid repo results in IFluidObject typing incompatibility Aug 18, 2020

This was referenced Aug 19, 2020

Remove certain "internal" interfaces from IFluidObject #3264

Closed

Demonstrate replacing IFluidObject with unknown and using queryObject fn for feature detection #3238

Closed

markfields mentioned this issue Aug 20, 2020

Refactor IFluidObject into a few different types for different parts of the feature detection scenario #3275

Closed

danielroney modified the milestones: August 2020, September 2020 Sep 3, 2020

markfields self-assigned this Sep 8, 2020

curtisman added api and removed bug Something isn't working labels Sep 22, 2020

anthony-murphy modified the milestones: September 2020, October 2020 Sep 28, 2020

anthony-murphy mentioned this issue Sep 30, 2020

Remove Shared Object FluidObject Interfaces #3782

Merged

curtisman modified the milestones: October 2020, Next Oct 2, 2020

vladsud mentioned this issue May 28, 2021

Collection of 1.0 items to consider #6272

Closed

61 tasks

ghost added the status: stale label Jun 8, 2021

ghost removed the status: stale label Jun 8, 2021

anthony-murphy mentioned this issue Jul 21, 2021

Layer Crossing Definitions Package Versioning and Architecture #6540

Closed

anthony-murphy modified the milestones: Next, August 2021 Aug 2, 2021

markfields removed their assignment Aug 2, 2021

anthony-murphy modified the milestones: August 2021, October 2021 Sep 1, 2021

anthony-murphy mentioned this issue Sep 28, 2021

IFluidObject Alternative Without Module Augmentation #7613

Closed

13 tasks

danielroney mentioned this issue Oct 20, 2021

IFluidObject cleanup #7920

Closed

3 tasks

anthony-murphy modified the milestones: October 2021, November 2021 Oct 26, 2021

anthony-murphy closed this as completed Nov 1, 2021

anthony-murphy mentioned this issue Nov 3, 2021

Add FluidObject utility type to Replace IFluidObject #8087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Almost any interface change in the Fluid repo results in IFluidObject typing incompatibility #2566

Almost any interface change in the Fluid repo results in IFluidObject typing incompatibility #2566

leeviana commented Jun 16, 2020 •

edited by anthony-murphy

Loading

leeviana commented Jun 16, 2020

markfields commented Aug 18, 2020

markfields commented Aug 19, 2020

markfields commented Aug 19, 2020 •

edited

Loading

markfields commented Aug 19, 2020

leeviana commented Aug 19, 2020 •

edited

Loading

leeviana commented Aug 19, 2020 •

edited

Loading

markfields commented Aug 20, 2020 •

edited

Loading

DLehenbauer commented Sep 22, 2020

curtisman commented Sep 22, 2020 •

edited

Loading

leeviana commented Sep 22, 2020

DLehenbauer commented Sep 22, 2020 •

edited

Loading

ghost commented Jun 8, 2021

leeviana commented Jun 8, 2021

markfields commented Aug 2, 2021

Stevenic commented Oct 20, 2021

anthony-murphy commented Oct 20, 2021

anthony-murphy commented Nov 1, 2021

Almost any interface change in the Fluid repo results in IFluidObject typing incompatibility #2566

Almost any interface change in the Fluid repo results in IFluidObject typing incompatibility #2566

Comments

leeviana commented Jun 16, 2020 • edited by anthony-murphy Loading

Describe the bug

To Reproduce

Expected behavior

leeviana commented Jun 16, 2020

markfields commented Aug 18, 2020

markfields commented Aug 19, 2020

markfields commented Aug 19, 2020 • edited Loading

markfields commented Aug 19, 2020

leeviana commented Aug 19, 2020 • edited Loading

leeviana commented Aug 19, 2020 • edited Loading

markfields commented Aug 20, 2020 • edited Loading

DLehenbauer commented Sep 22, 2020

curtisman commented Sep 22, 2020 • edited Loading

leeviana commented Sep 22, 2020

DLehenbauer commented Sep 22, 2020 • edited Loading

ghost commented Jun 8, 2021

leeviana commented Jun 8, 2021

markfields commented Aug 2, 2021

Stevenic commented Oct 20, 2021

anthony-murphy commented Oct 20, 2021

anthony-murphy commented Nov 1, 2021

leeviana commented Jun 16, 2020 •

edited by anthony-murphy

Loading

markfields commented Aug 19, 2020 •

edited

Loading

leeviana commented Aug 19, 2020 •

edited

Loading

leeviana commented Aug 19, 2020 •

edited

Loading

markfields commented Aug 20, 2020 •

edited

Loading

curtisman commented Sep 22, 2020 •

edited

Loading

DLehenbauer commented Sep 22, 2020 •

edited

Loading