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.

Sign up for GitHub

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

Jump to bottom

Updated index.d.ts with ResourceType to ResourceTypes #468

Merged
merged 4 commits into from
Aug 20, 2019
Merged

Updated index.d.ts with ResourceType to ResourceTypes #468

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dad3292
Updated index.d.ts with ResourceType to ResourceTypes
wmtst8 Aug 14, 2019
53c88bb
Reverted ResourceTypes to ResourceType in types/index.d.ts and added …
wmtst8 Aug 15, 2019
40cf2fb
Fix ResourceTypes typescript definition
tulios Aug 20, 2019
e16635b
Merge branch 'master' into fix/d-ts-resource-type
tulios Aug 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ export interface ITopicMetadata {
partitions: PartitionMetadata[]
}

export enum ResourceType {
export enum ResourceTypes {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is backward-incompatible, we will have to keep both names for a while :/

Copy link
Contributor Author

@wmtst8 wmtst8 Aug 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. I'll revert to ResourceType and add a second enum named ResourceTypes for now. Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need both? the .d.ts is bundled with the kafkajs version itself, so it only needs to support that particular version of kafkajs. And in the latest version kafkajs is using ResourceTypes it seems according to docs/Admin.md.

const { ResourceTypes } = require('kafkajs')

await admin.describeConfigs({
  includeSynonyms: false,
  resources: [
    {
      type: ResourceTypes.TOPIC,
      name: 'topic-name'
    }
  ]
})

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens at runtime doesn't really matter, since the two types are equivalent anyway. The issue is how to make this change without people updating KafkaJS and finding that their build no longer works.

Just adding a second Enum with the correct name doesn't do anything. What we would want is to be able to add a new Enum and update all the references to return that, and then keep the old Enum around in such a way that old code will still compile.

I'm far from a TS expert, but I would expect that we would need to update the references to now be a union of ResourceTypes and ResourceType:

export enum ResourceType {
  UNKNOWN = 0,
  ANY = 1,
  TOPIC = 2,
  GROUP = 3,
  CLUSTER = 4,
  TRANSACTIONAL_ID = 5,
  DELEGATION_TOKEN = 6,
}

// wouldn't it be great if TS allowed you to extend string-based enums...
export enum ResourceTypes {
  UNKNOWN = 0,
  ANY = 1,
  TOPIC = 2,
  GROUP = 3,
  CLUSTER = 4,
  TRANSACTIONAL_ID = 5,
  DELEGATION_TOKEN = 6,
}

export interface ResourceConfigQuery {
  type: ResourceType | ResourceTypes
  name: string
  configNames: string[]
}

export interface DescribeConfigResponse {
  resources: {
    configEntries: ConfigEntries[]
    errorCode: number
    errorMessage: string
    resourceName: string
    resourceType: ResourceTypes | ResourceType
  }[]
  throttleTime: number
}

Copy link
Contributor

@sklose sklose Aug 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the current enum is just broken with TypeScript because the .d.ts file is just for type definitions. Everything in there still needs some backing implementation that is available at runtime. If you do

import { ResourceType } from "kafkajs";
console.log(ResourceType.DELEGATION_TOKEN);

you will get

console.log(kafkajs_1.ResourceType.DELEGATION_TOKEN);
                                   ^
TypeError: Cannot read property 'DELEGATION_TOKEN' of undefined
    at Object.<anonymous> (/Users/s0k00sg/Work/sandbox/dist/kafkajs.js:26:36)
    at Module._compile (module.js:643:30)
    at Object.Module._extensions..js (module.js:654:10)
    at Module.load (module.js:556:32)
    at tryModuleLoad (module.js:499:12)
    at Function.Module._load (module.js:491:3)
    at Function.Module.runMain (module.js:684:10)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:608:3

because this will transpile into the following JavaScript:

const kafkajs_1 = require("kafkajs");
console.log(kafkajs_1.ResourceType.DELEGATION_TOKEN);

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, @sklose is right. The code in the .d.ts has to match what's in the JS parts. I will fix the file and merge the PR, thanks.

UNKNOWN = 0,
ANY = 1,
TOPIC = 2,
Expand All @@ -193,7 +193,7 @@ export enum ResourceType {
}

export interface ResourceConfigQuery {
type: ResourceType
type: ResourceTypes
name: string
configNames: string[]
}
Expand All @@ -219,13 +219,13 @@ export interface DescribeConfigResponse {
errorCode: number
errorMessage: string
resourceName: string
resourceType: ResourceType
resourceType: ResourceTypes
}[]
throttleTime: number
}

export interface IResourceConfig {
type: ResourceType
type: ResourceTypes
name: string
configEntries: { name: string; value: string }[]
}
Expand Down