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

Incorrect Kafka Initialization Example in README Causing TypeScript Error #225

Open
archepro84 opened this issue Dec 28, 2024 · 2 comments
Labels
documentation Improvements or additions to documentation

Comments

@archepro84
Copy link

Environment Information

  • OS [e.g. Mac, Arch, Windows 10]: Mac
  • Node Version [e.g. 8.2.1]: 22.12.0
  • NPM Version [e.g. 5.4.2]: 10.9.0
  • C++ Toolchain [e.g. Visual Studio, llvm, g++]: -
  • confluent-kafka-javascript version [e.g. 2.3.3]: 1.0.0

Overview

To connect to Kafka, the class Kafka from ('@confluentinc/kafka-javascript').KafkaJS is used to manage connections and create Producers and Consumers. However, incorrect initialization methods are presented in the README and some example codes, causing errors.

Problematic Code

Extracted and slightly modified from the README and Confluent Kafka Node.js Client examples

import {KafkaJS} from "@confluentinc/kafka-javascript";

const {Kafka} = KafkaJS;

async function produce(config?: ProducerConstructorConfig): Promise<void> {
// create a new producer instance
  const producer = new Kafka().producer(config);
}

async function consume(config: ConsumerConstructorConfig) {
  const consumer = new Kafka().consumer(config);
}

Error Message

TS2554: Expected 1 arguments, but got 0

kafkajs.d.ts(97, 15): An argument for config was not provided.

The class Kafka requires CommonConstructorConfig as a mandatory argument in its constructor. However, the examples
omit this argument, leading to errors.

Cause Analysis

CommonConstructorConfig is a mandatory parameter that defines the detailed connection settings for the Kafka cluster. While omitting this argument does not cause issues in JavaScript, it results in a type error in TypeScript.

Proposed Solutions

  1. Make CommonConstructorConfig Optional in class Kafka
    • Pros: Ensures compatibility with the existing example code and prevents type errors.
    • Cons: It may make it difficult to validate the configuration at the higher code level. Additionally, this pattern is not adopted by libraries like KafkaJS or node-rdkafka.
  2. Modify the Example Code to Use new Kafka({})
    • Pros: The problem can be resolved by updating the examples without changing the codebase.
    • Some examples in the repository and the migration guide already use this approach. This ensures consistent conventions without requiring code-level changes.
    • Cons: May not be backward-compatible with existing users. Additionally, it is necessary to verify if this
      approach aligns with the philosophy of the library.

Additional Questions

The confluent-kafka-javascript library appears to be designed based on node-rdkafka and KafkaJS. Is it a long-term goal to migrate all functionalities of KafkaJS into this library?

@milindl
Copy link
Contributor

milindl commented Jan 2, 2025

Thanks for filing this! I'll probably make CommonConstructorConfig optional as we are okay with someone providing the entire config in the producer({...}) call.

Regarding the other question, we have migrated most of what we targeted to migrate from KafkaJS. Depending on user feedback, we might consider some API additions or changes, but right now, this is pretty much it in terms of the migration.

A few exceptions are the AdminAPI (we plan to add more of them), and certain configuration values (like the size of the batch in the eachBatch callback).

Besides this, we won't migrate any future additions to the KafkaJS library (or node-rdkafka for that matter), and there are certain things we've chosen not to migrate (for example, the .on events). A more complete list is here: MIGRATION.md.

@milindl milindl added the documentation Improvements or additions to documentation label Jan 2, 2025
@archepro84
Copy link
Author

Thank you so much for the quick response! 😊

Thanks for filing this! I'll probably make CommonConstructorConfig optional as we are okay with someone providing the entire config in the producer({...}) call.

I think making CommonConstructorConfig optional to allow flexibility with configurations for librdkafka or KafkaJS is an excellent direction.
However, if both Kafka and Producer configurations are defined as optional, it might cause some confusion for developers, as it becomes unclear which settings are essential.

Would it be possible to mitigate this confusion by explicitly defining the minimum required fields or providing default values in the examples?

Besides this, we won't migrate any future additions to the KafkaJS library (or node-rdkafka for that matter), and there are certain things we've chosen not to migrate (for example, the .on events). A more complete list is here: MIGRATION.md.

I am currently developing an application using Nest.js and KafkaJS. Part of this involves handling lifecycle events with kafka.on() through the EventEmitter to process library-level events.
It’s a bit unfortunate to hear that this feature won’t be supported in the migration.

Could you share the reasoning behind excluding this feature from the migration plan? Understanding the background would help us find better alternatives to adapt our application. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

2 participants