js-xdr icon indicating copy to clipboard operation
js-xdr copied to clipboard

Add a robustier `instanceof` alternative to allow cross-package compatibility.

Open Shaptic opened this issue 1 year ago • 1 comments

Note: Lots of people tagged for review mostly as an FYI as there may or may not be far-reaching implications to this change.

What

Introduce a better check for cross-compatible XdrCompositeType subtypes (Union, Struct, and Array) based on a Discord discussion from @earrietadev which will check the prototype chain for the constructors and properties we need:

export function isSerializableIsh(
    value: any, 
    subtype: InstanceType<typeof XdrCompositeType>
): value is XdrCompositeType;

(This isn't a proper TypeScript definition because js-xdr (a) doesn't use TS and (b) doesn't use interfaces, but imagine XdrCompositeType as being an interface and it makes more sense.)

Why

Due to an inability to root cause current issues with the TypeScript bindings alongside the Stellar SDK (see https://github.com/stellar/soroban-cli/issues/1285, https://github.com/stellar/js-soroban-client/issues/124, and many Discord threads like this one), we are forced to try alternatives.

It is also technically true that major versions of a package should probably be cross-compatible in the same dependency tree. For example, [email protected] should function interchangeably alongside [email protected] if the additive features are not relevant.

That is not the case today due to instanceof checks that become package-specific, while isSerializableIsh should rectify this.

Known Limitations

I am worried and wonder that this is treating symptoms rather than causes (given that all cases of this error that have been root-caused are related to actual issues in e.g. user error, packaging structure, or mixing dependencies). However, the "technically true" bit from the Why still applies here.

There are only performance implications for people who are introducing this mixing behavior (besides the new overhead of a function call which I would consider trivial), so there are no perf regression risks.

Shaptic avatar May 09 '24 20:05 Shaptic

The new check also fails to resolve https://github.com/stellar/soroban-cli/issues/1285

BlaineHeffron avatar May 09 '24 21:05 BlaineHeffron

This looks solid to me. I'll echo @willemneal 's thought about using the Class rather than this. Probably not super critical, but you may as well cover all your bases

piyalbasu avatar Jun 25 '24 15:06 piyalbasu

I've tried testing this with downstream systems and landed upon the wonderfully helpful "[object Object]'s struct name is SorobanAuthorizationEntry not SorobanAuthorizationEntry: <a JSON dump of the object>" so clearly the extra check is still failing in key moments. I'm still in the process of debugging it and will merge this once I can pull that off.

Shaptic avatar Jul 01 '24 20:07 Shaptic

Yooooo I ran this through a project that uses vite all the way down through the SDK and it works!! :partying_face: :partying_face:

Shaptic avatar Jul 17 '24 22:07 Shaptic