cph-cachet / carp.core-kotlin

Infrastructure-agnostic framework for distributed data collection.
https://carp.cachet.dk/core/
MIT License
21 stars 3 forks source link

TypeScript export for `Nullable` is missing #443

Closed Whathecode closed 1 year ago

Whathecode commented 1 year ago

Nullable is used in the generated TypeScript API, but not exported.

@SlimShadyIAm ran into this and provided the following use case:

image

  setParticipantData_CORE = async (
    studyDeploymentId: string,
    data: HashMap<NamespacedId, Data | null>,
    inputType: string | null,
    config: AxiosRequestConfig
  ): Promise<ParticipantData> => {
    try {
      const participantDataRequest = new ParticipationServiceRequest.SetParticipantData(new UUID(studyDeploymentId), data, inputType);

It was just complaining that the type of the data parameter doesn't match what SetParticipantData expects

Whathecode commented 1 year ago

@SlimShadyIAm What exactly doesn't work? This works just fine for me:

            const inputByParticipantRole: string | null = null
            const setParticipantData = new ParticipationServiceRequest.SetParticipantData(
                UUID.Companion.randomUUID(),
                mapOf( [] ),
                inputByParticipantRole
            )
SlimShadyIAm commented 1 year ago

So in the example you've shown in your latest comment, you've put in a map with an empty array for the data parameter. The code example you attached in your OP was an extract from the httpclient, and since it's a wrapper around the API endpoint I would like to pass in some actual data for the request.

The problem is that originally the type definition for data was HashMap<NamespacedId, Data | null>, but in the new types it has changed to HashMap<NamespacedId, Nullable<Data>> and the Nullable type hasn't been exported, so I can't add the correct type definition for the parameter in my function.

With the example you added in your latest comment, I tried doing mapOf([data]) when data is of type Data...

  setParticipantData_CORE = async (
    studyDeploymentId: string,
    data: Data,
    inputType: string | null,
    config: AxiosRequestConfig
  ): Promise<ParticipantData> => {
    try {
      const participantDataRequest = new ParticipationServiceRequest.SetParticipantData(new UUID(studyDeploymentId), mapOf([data]), inputType);

...but that also doesn't work because mapOf still expects Pair<NamespacedId, Nullable<Data>>.

When we originally talked about this issue I remember you saying you just needed to manually export the Nullable type like you had done for some others.

Whathecode commented 1 year ago

...but that also doesn't work because mapOf still expects Pair<NamespacedId, Nullable>.

Yes. The API docs may need some polishing. It is a map, with keys defining "which data" is being set, identified by NamespacedId (in Kotlin InputDataType is just an alias for this), and the value being the actual Data. Pair's are exported in TypeScript declarations; check some examples here.

When we originally talked about this issue I remember you saying you just needed to manually export the Nullable type like you had done for some others.

Maybe. I was just adding a test replicating your issue, and couldn't. I'll try again with this additional context. But the reason it doesn't seem needed is because the TypeScript type system is flexible enough to accept SomeType | null for Nullable<SomeType> arguments; it doesn't complain because of duck typing. The third parameter is also a Nullable<string>, but you can see I just pass a string | null.

SlimShadyIAm commented 1 year ago

the TypeScript type system is flexible enough to accept SomeType | null for Nullable<SomeType> arguments; it doesn't complain because of duck typing.

I would have thought so as well, you're right that it doesn't complain about the third parameter being string | null so it's a bit strange.

Also, my point about Pair was that I would still need Nullable to do it that way because of the same issue.

Here's an example of something we have in our tests for the httpclient that was previously used for this endpoint: const participantData = new nameSpaceId('dk.cachet.carp.input.sex','male' );. Could you maybe try to extend your test with this as the data?

Whathecode commented 1 year ago

@SlimShadyIAm Updated the test to pass actual data, as defined by the type system: https://github.com/cph-cachet/carp.core-kotlin/commit/01b6fbcc85f0db291fccc8120cbc1ed03b05315a

Works just fine:

    describe( "ParticipationServiceRequest", () => {
        it( "can initialize SetParticipantData", () => {
            const inputByParticipantRole: string | null = null
            const setParticipantData = new ParticipationServiceRequest.SetParticipantData(
                UUID.Companion.randomUUID(),
                mapOf( [ new Pair( CarpInputDataTypes.SEX, Sex.Male ) ] ),
                inputByParticipantRole
            )
        } )
    } )

I'm gonna guess the original wrapper was wrong before, and if it "worked" before, it didn't work properly. The above is what is expected by the backend. You probably want to thoroughly rework all the wrappers now that full type info is available.

If the wrappers are needed at all, ... I never was certain why there are there. 🙂

SlimShadyIAm commented 1 year ago

@Whathecode Sorry for the late response on this.

I've tried again and unfortunately I'm still struggling with this. In my test...

      test.skip('setParticipantData should succeed', async () => {
        const inputByParticipantRole: string | null = null;
        const setParticipantData = mapOf([new Pair(CarpInputDataTypes.SEX, Sex.Male)]);
        const response = await carpInstance.setParticipantData_CORE(
          studyDeploymentId.stringRepresentation,
          setParticipantData, // Argument of type 'Map<NamespacedId, Sex & { readonly name: "Male"; readonly ordinal: 0; }>' is not assignable to parameter of type 'Map<NamespacedId, Data | null>'.
          inputByParticipantRole,
          config
        );
        expect(response instanceof Ok).toBeTruthy();
      });

...and the wrapper setParticipantData_CORE; the problem is actually here, because I'm trying to make the data parameter generic so that you could pass anything into it, as opposed to your test where you're directly passing the data into the request constructor.

   setParticipantData_CORE = async (
    studyDeploymentId: string,
    data: HashMap<NamespacedId, Data | null>,
    inputType: string | null,
    config: AxiosRequestConfig
  ): Promise<ParticipantData> => {
    try {
      const participantDataRequest = new ParticipationServiceRequest.SetParticipantData(new UUID(studyDeploymentId), data, inputType); // Argument of type 'Map<NamespacedId, Data | null>' is not assignable to parameter of type 'Map<NamespacedId, Nullable<Data>>'.
      const json: Json = DefaultSerializer;
      const serializer = ParticipationServiceRequest.Serializer;
      const serializedRequest = json.encodeToString(serializer, participantDataRequest);
      const response = await this._instance.post('/api/participation-service', serializedRequest, config);
      return Promise.resolve(response.data as ParticipantData);
    } catch (error) {
      return Promise.reject(unwrapError(error, 'setting participant data failed').value);
    }
  };

(Note that I've added the type errors where I see them)

Whathecode commented 1 year ago

If this is still relevant ... I still can't see the problem you are running into:

            function test( arg: Map<NamespacedId, Data | null> ): Map<NamespacedId, Data | null>
            {
                return arg
            }

            const inputByParticipantRole: string | null = null
            const data: Map<NamespacedId, Data | null> = mapOf( [ new Pair( CarpInputDataTypes.SEX, Sex.Male ) ] )
            const setParticipantData = new ParticipationServiceRequest.SetParticipantData(
                UUID.Companion.randomUUID(),
                test( data ),
                inputByParticipantRole
            )

I do, however, see that you are using data: HashMap<NamespacedId, Data | null>, and HashMap is no longer exported in the new declarations afaik.

Are you using the correct carp imports as per the example?

import { kotlin } from '@cachet/carp-kotlin'
import mapOf = kotlin.collections.mapOf
import Map = kotlin.collections.Map
import Pair = kotlin.Pair

import { dk } from '@cachet/carp-deployments-core'

import common = dk.cachet.carp.common
import Data = common.application.data.Data
import NamespacedId = common.application.NamespacedId
import UUID = common.application.UUID
import CarpInputDataTypes = common.application.data.input.CarpInputDataTypes
import Sex = common.application.data.input.Sex

import deployments = dk.cachet.carp.deployments
import ParticipationServiceRequest = deployments.infrastructure.ParticipationServiceRequest