search

peterprib / node-red-contrib-kafka-manager

Implement
GNU General Public License v3.0
22 stars 12 forks source link

TypeError: done is not a function in KafkaBroker #41

Closed chudesno closed 2 years ago

chudesno commented 2 years ago

When redeploying a flow error TypeError: done is not a function appears in the Debug messages. It looks like it originates from KafkaBrokerNode.close at kafkaManager/kafkaBroker.js:384:35

It appears that nodered runtime itself defines that method with a different interface here:

/**
 * Called when the node is being stopped
 * @param  {boolean} removed Whether the node has been removed, or just being stopped
 * @return {Promise} resolves when the node has closed
 */
Node.prototype.close = function(removed) {...}

May the method be renamed?

peterprib commented 2 years ago

Looking at the code , done is not used at 384 but can see reference to close definition at 374 this.close = function (removed, done) {

The code you have included, doesn't have the "done" parameter which makes me concern that the underlying code has been changed somehow incorrectly by some process. Not sure how renaming solves anything. Renaming "this" to "node" could fix the issue, but not sure why it should have an impact as about context.

It implies the code base has been regressed or ungraded and the async feature no longer exists. The later sounds wrong as value add.

Need more information to properly resolve the issue. Need to see extract of log which details problem and release information at start.

Likely fix would be "if(done) done()" a few line later.

see https://nodered.org/docs/creating-nodes/node-js#closing-the-node

peterprib commented 2 years ago

assume issue is related to version of node-red being old

chudesno commented 2 years ago

Yes, now I see that node-red-contrib-kafka-manager depends on node-red@^1.2.2 while I tried to use it with version 2+. Getting the same error when using official node-red docker image having NR v2.2.0.

Do you have any plans on supporting later versions or is it out of scope for the time being?

peterprib commented 2 years ago

surprise that it doesn't work on last version as doco implies it does. Thought I had used on recent release so will take a look when I get a chance.

chudesno commented 2 years ago

For the clarity, I have not seen any issues with the functionality itself. This is just an error message appearing in debug messages panel on deploy. Many thanks for looking into this, @peterprib !

peterprib commented 2 years ago

based on where occurring should have no material impact. Will still fix. Be interesting to understand why this call no longer aligns with documentation. Have seen the message in v2.2