Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeScript export for Nullable is missing #443

Closed
Whathecode opened this issue May 25, 2023 · 7 comments · Fixed by #445
Closed

TypeScript export for Nullable is missing #443

Whathecode opened this issue May 25, 2023 · 7 comments · Fixed by #445
Labels
bug Something isn't working the way it was designed to.

Comments

@Whathecode
Copy link
Member

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 Whathecode added the bug Something isn't working the way it was designed to. label May 25, 2023
@Whathecode
Copy link
Member Author

Whathecode commented Jun 23, 2023

@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
Copy link

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
Copy link
Member Author

Whathecode commented Jun 30, 2023

...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
Copy link

SlimShadyIAm commented Jun 30, 2023

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
Copy link
Member Author

Whathecode commented Jul 22, 2023

@SlimShadyIAm Updated the test to pass actual data, as defined by the type system: 01b6fbc

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
Copy link

SlimShadyIAm commented Aug 14, 2023

@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
Copy link
Member Author

Whathecode commented Oct 11, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working the way it was designed to.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants