Dedup definitions of TJSON type

[jberdine]

The 2nd commit of this PR does not type-check in a way that confuses me. The error is:

src/internals/skipruntime_module.ts:1367:12 - error TS2352: Conversion of type 'string | boolean | JSONObject | (TJSON | null)[] | ObjectProxy<{ [k: string]: Exportable; }> | ArrayProxy<any> | null | undefined' to type 'GetResult<Values<K, V>>' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  Type 'ArrayProxy<any>' is missing the following properties from type 'GetResult<Values<K, V>>': payload, errors

1367     return result as GetResult<Values<K, V>>;
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This seems to have nothing to do with removing the bigint alternative from TJSON. Additionally, the result as expression does not make sense to me, as in context the type of result is Exportable which does not seem to have any relation to GetResult. Does anyone have a hint for what is going on here?

Another question is whether bigint is intended to be included in TJSON since it appears in one definition but not others.

[jberdine]

Cc @skiplabsdaniel and @bennostein as this code comes from https://github.com/SkipLabs/skip/pull/425.

[bennostein]

Another question is whether bigint is intended to be included in TJSON since it appears in one definition but not others.

Taking a look now at the main question, but for this one: no, bigint should not be included in the skipruntime_api TJSON type, since it's not directly serializable to JSON in general -- big ints get encoded as strings.

[jberdine]

Another question is whether bigint is intended to be included in TJSON since it appears in one definition but not others.

Taking a look now at the main question, but for this one: no, bigint should not be included in the skipruntime_api TJSON type, since it's not directly serializable to JSON in general -- big ints get encoded as strings.

And AFAIU "it would be bad" if skjson and skipruntime_api used different/incompatible definitions of TJSON.

[bennostein]

And AFAIU "it would be bad" if skjson and skipruntime_api used different/incompatible definitions of TJSON.

Can you elaborate? I'm trying to determine now whether that's true, as I vaguely recall Daniel saying that the difference was intentional/necessary.

[skiplabsdaniel]

The good definition is the api one, the bigint appears by mistake.

[jberdine]

And AFAIU "it would be bad" if skjson and skipruntime_api used different/incompatible definitions of TJSON.

Can you elaborate? I'm trying to determine now whether that's true, as I vaguely recall Daniel saying that the difference was intentional/necessary.

I have not dug in in great detail, but as far as I have understood, there are uses of TJSON all over and it is unclear (to me) why skjson should support bigint but nothing else should. If there is an intentional/needed difference, one of the two types should be renamed so we can keep track of where bigint is supported and where not.

[jberdine]

The good definition is the api one, the bigint appears by mistake.

Thanks Daniel. This makes the question about the GetResult cast more important since removing the bigint alternative breaks the type-checker. :-/

[skiplabsdaniel]

Type type checker is right, the result cannot be directly a GetResult<Values<K, V>> as Values<K, V> have a ReactiveResponse with a bigint.

[skiplabsdaniel]

I'm not sure we need to deduplicate now, skjson's TJSON corresponds to the wasm chain, skipruntime's TJSON is a typescript chain that will work just as well with wasm chain as with the native chain. So it would be a shame to force dependencies from the wasm chain into the native chain.

[jberdine]

Ok, I won't go ahead with this. I did not realize this would impact native dependencies. For my understanding, would the use of the skjson definition in core/src/skipruntime_api.ts be fine since core/package.json already has a dependency on skjson, but it is the use of the skjson definition in client/src/protocol.ts that is a problem since client/package.json does not have a dependency on skjson?

I still think it would be good to have the definition (of each type) only once somewhere that is dependency-friendly.

[skiplabsdaniel]

It's true that the current state is confusing because no native chain exist. The PR #443 separates more the packages to manage correctly both chain. But I agree we need to have the definition of each type once.