CardanoSolutions / ogmios

❇️ A WebSocket JSON/RPC bridge for Cardano
https://ogmios.dev
Mozilla Public License 2.0
304 stars 90 forks source link

Schema type definitions shouldn't be optional #41

Closed MarcelKlammer closed 3 years ago

MarcelKlammer commented 3 years ago

Describe what the problem is?

The generated schema types, which are used to define expected types in code, shouldn't be optional as in

RequestNext?: ...

As those types are indeed defined right after the :

The ? 'confuses' auto completion and parsers, eg. result is not a property of RequestNext | undefined.


export interface Ogmios {
    RequestNext?: {
        type: "jsonwsp/request";
        version: "1.0";
        servicename: "ogmios";
        methodname: "RequestNext";
        args?: {};
        mirror?: {
            [k: string]: unknown;
        };
    };
...
}

What should be the expected behavior?

After removing all ? from the Ogmios types the parsers actually work.

KtorZ commented 3 years ago

@rhyslbw Any idea here?

rhyslbw commented 3 years ago

I think I'm missing some context here @MarcelKlammer. What do you mean by "the parsers actually work"? Can you paste a sample of your code so I can see this in context?

MarcelKlammer commented 3 years ago

export interface Ogmios {

...

    "QueryResponse[stakeDistribution]"?: {
        type: "jsonwsp/response";
        version: "1.0";
        servicename: "ogmios";
        methodname: "Query";
        result: PoolDistribution | EraMismatch | QueryUnavailableInCurrentEra;
        reflection?: {
            [k: string]: unknown;
        };
    };

...
}

export const isEraMismatch = (result: Ogmios['QueryResponse[stakeDistribution]']['result']): result is EraMismatch =>
  (result as EraMismatch).eraMismatch !== undefined

TypeScript checks each type annotation.

The type "QueryResponse[stakeDistribution]"? is an "optional" type, it's short for "QueryResponse[stakeDistribution]" | undefined.

So my IDE complains about Ogmios['QueryResponse[stakeDistribution]']['result'], because indeed result is not a property of the joined type: QueryResponse[stakeDistribution]" | undefined

Removing the ? will fix it, then it is of type "QueryResponse[stakeDistribution]", which has result as property.

So, in short, please remove the ? on every type in interface Ogmios.

rhyslbw commented 3 years ago

The type definitions are derived from the JSON-WSP schema, so either the interpretation of the schema needs to change in the generator, or it's a condition we must deal with. There are some potential changes with the JSON-WSP schema to explore, but have you tried using the @cardano-ogmios/client package? It should shield you from the particular issue you're raising with user-defined type guards along with existence checks within the client transformations.

MarcelKlammer commented 3 years ago

I prefer implementing against the socket api myself. Nothing against your code, but I don't like the "code flow".

maybe a

, "required": [ "QueryResponse[stakeDistribution]", ... ] 

including all types right at the top would get rid of the trailing '?' ?

(That's just a wild guess)

rhyslbw commented 3 years ago

No problem, that's why the schema is in a separate package :slightly_smiling_face: I'm also all for improving the generated output, as it will reduce the complexity in the client package too.

including all types right at the top would get rid of the trailing '?' ?

Should be easy enough to test. Are you building from source, or using the package from the registry? FYI the latter is out of date as we've not setup the workflow to publish releases yet

https://github.com/KtorZ/cardano-ogmios/blob/3fb1c139f5f25ae96d6a8884f538a572e31a4317/clients/TypeScript/package.json#L19

MarcelKlammer commented 3 years ago

Yes, adding a required field with the type will result in non optional definitions.

, "required": [
  "RequestNext", "RequestNextResponse",
  "FindIntersect", "FindIntersectResponse",
  "SubmitTx", "SubmitTxResponse",
  "Acquire", "AcquireResponse",
  "Release","ReleaseResponse",
  "Query",
  "QueryResponse[eraStart]",
  "QueryResponse[ledgerTip]",
  "QueryResponse[currentEpoch]",
  "QueryResponse[nonMyopicMemberRewards]",
  "QueryResponse[delegationsAndRewards]",
  "QueryResponse[currentProtocolParameters]",
  "QueryResponse[proposedProtocolParameters]",
  "QueryResponse[stakeDistribution]",
  "QueryResponse[utxo]",
  "QueryResponse[genesisConfig]",
  "Fault"
]
rhyslbw commented 3 years ago

Reopening to use as task for TypeScript client update

KtorZ commented 3 years ago

@rhyslbw I am still unsure what you want to do with that one :sweat_smile: ? As far as I am concerned, the TypeScript type definitions are okay and this issue can be closed? If there's something specific that needs doing, I'd suggest to open a new ticket about that specific item.

rhyslbw commented 3 years ago

Yes, pretty sure this is now obsolete @KtorZ