node-slack-sdk icon indicating copy to clipboard operation
node-slack-sdk copied to clipboard

Type generation creates UseridMap as an interface when it is a Map/Record/object

Open rockingskier opened this issue 2 years ago • 3 comments

When generating types for packages/web-api/src/response/MigrationExchangeResponse.ts, UseridMap is created as an interface when really it should be a Record.

This means when calling migration.exchange the data in response.user_id_map needs to be cast before it can used.

// Warning: pseudo code alert
type UserIds = Partial<Record<string, string>>;

const migrateUserIds = (userIds: string[]): UserIds => {
  const response = await client.migration.exchange({
    team_id: "WORKSPACEID",
    users: userIds.join(","),
  });
  
  if (!response.ok) return {};
  return return response.user_id_map as UserIds;  // Mmmmm
}

Packages:

Select all that apply:

  • [x] @slack/web-api
  • [ ] @slack/rtm-api
  • [ ] @slack/webhooks
  • [ ] @slack/oauth
  • [ ] @slack/socket-mode
  • [ ] @slack/types
  • [ ] I don't know

Reproducible in:

Whatever is in main at the moment: 57dfddaa5d1442aa3f854f8b51895b0d9c04e63c

Steps to reproduce:

./generate-web-api-types.sh

Expected result:

# packages/web-api/src/response/MigrationExchangeResponse.ts
// ...
export type UseridMap  = Record<string, string>  // Should probably be a `Partial` or `string | undefined`

Actual result:

# packages/web-api/src/response/MigrationExchangeResponse.ts
// ...
export interface UseridMap {
}

rockingskier avatar Jan 24 '23 00:01 rockingskier

Hi @rockingskier, thanks for taking the time to report this! I agree that this should be improved in future versions

seratch avatar Jan 24 '23 00:01 seratch

I've been having a dig around...

This is the Java class: https://github.com/slackapi/java-slack-sdk/blob/main/slack-api-client/src/main/java/com/slack/api/methods/response/migration/MigrationExchangeResponse.java#L37 This is the example json: https://github.com/slackapi/java-slack-sdk/blob/main/json-logs/samples/api/migration.exchange.json#L12

The generator uses quicktype to convert the JSON to TS. It uses "the dark arts" (Markov chain - https://blog.quicktype.io/markov/) to decide when an object should be an interface or a map.

If quicktype thinks your object is a class when it should be a map, a bit more work is needed. The easiest way to correct this is to trick quicktype by making the property names look "weird":

I dropped the example JSON into https://app.quicktype.io/ and had a quick go and tricking quicktype but had no luck.

rockingskier avatar Jan 24 '23 01:01 rockingskier

I haven't checked in detail yet, but the approach I took for the "shares" property may work for this too:

  • https://github.com/slackapi/java-slack-sdk/blob/v1.27.3/slack-api-client/src/test/java/util/sample_json_generation/SampleObjects.java#L286-L287
  • https://github.com/slackapi/java-slack-sdk/blob/v1.27.3/json-logs/samples/api/search.messages.json#L566-L581
  • https://github.com/slackapi/node-slack-sdk/blob/%40slack/web-api%406.8.0/packages/web-api/src/response/SearchMessagesResponse.ts#L490-L493

seratch avatar Jan 24 '23 01:01 seratch