Task Handler arrow function property names are sometimes an empty string

[tegefaulkes]

Specification

In the NodeManager we make use of 3 handlers with the task system. Their handler IDs are public read only properties that are public readonly handlerId = '${this.constructor.name}.${this.handler.name}' as HandlerId. this.handler is an async arrow function used as a handler for a task. The handlerId should be 'NodeManager.handler'.

The problem is, in some cases the this.handler.name has the value of '' as an empty string. The weird thing is, in a simple test where we just construct the NodeManager and check it we get the expected values. When we do more complex tests within the nodes domain we see the empty strings resulting in errors.

My first guess is it's the same decorator problem we had to fix for the this.constructor.name when we switched to SWC. I'm guessing when we start the NodeManager it overwrites the property names? More exploration on this is needed.

Additional context

• Related Integrate TaskManager into NodeGraph and Discovery #445
• Related comments ci: merge staging to master #411 (comment)

Tasks

1. Determine the cause of the problem.
2. Fix property names so the are not empty strings.

[CMCDragonkai]

Ok so with an async arrow function, one must be aware that the arrow function this does exist by default. Instead it is bound to the parent context.

Problem is... what is the "parent closure" of an arrow function? Well usually it is whatever is defining it.

But if you're defining it in a class... then this could mean what exactly?

Let's try this:

class X {
  static a () {
    console.log('1', this.constructor.name);
    console.log('2', this.name);
  }
  b = () => {
    console.log('3', this.constructor.name);
    // @ts-ignore
    console.log('4', this.name);
    console.log('5', this.c);
    console.log('6', this.b.name);
  };
  c = 3;
}

console.log(X.a());
const x = new X();
console.log(x.b());

Running it gives us:

1 Function
2 X
undefined
3 X
4 undefined
5 3
6 b

Therefore in static functions, the this is the class, while in the arrow function, the this is the instance.

So for this.handler.name to become an empty string, the only possibility is either due to compilation of tsc or swc. And possibly a use of a decorator, either class decorator or method/property decorator or a mix of these issues.

[CMCDragonkai]

My tests show that this shouldn't happen. With swc or otherwise.

If it does still show up... we may just change the handler name and hardcode it instead.

[CMCDragonkai]

I'm just thinking there could be an ordering issue with:

public readonly handlerId = '${this.constructor.name}.${this.handler.name}' as HandlerId

That seems like something that may just need to be inlined wherever it is used.

[CMCDragonkai]

@tegefaulkes I can't find anywhere in the code where we are doing this, as in setting it as a property. Is this issue even relevant anymore?

[CMCDragonkai]

Ok I see it like:

  public readonly checkSeedConnectionsHandlerId: TaskHandlerId =
    `${this.basePath}.${this.checkSeedConnectionsHandler.name}.checkSeedConnectionsHandler` as TaskHandlerId;

I think this can be remove in favour of inlining this sort of stuff. The whole idea is that his is supposed to be set when the instance is created. If you want to do this sort of stuff, prefer setting it in the constructor instead, or even just dynamically created in the functions themselves.

In fact the usage of public readonly properties that are somewhat dynamic seems kind of misuse, and can be easily confused.

[CMCDragonkai]

In keys/CertManager, this was done as:

keys/CertManager.ts
15:import type { CertId, TaskHandlerId, TaskId } from '../ids/types';
138:  protected renewCurrentCertHandlerId: TaskHandlerId =
139:    `${this.constructor.name}.renewCurrentCert` as TaskHandlerId;

So I think hardcoding it, with only this.constructor.name is better.

[CMCDragonkai]

This is a quick fix @tegefaulkes.

[tegefaulkes]

We're just going to hard-code the handler IDs now just to keep it simple. This should be resolved once we make this change to all handler IDs.

[CMCDragonkai]

Seems like constructor name works fine. But just hardcode the name of the handler.

[CMCDragonkai]

Should have this in the documentation too.

[CMCDragonkai]

@tegefaulkes if we have decided to just hardcode the handler IDs, then you can close this (technically a workaround, not a fix but whatever).

[CMCDragonkai]

But make sure it's applied all over the place. Should not have any inconsistency. Commit message should refer to this issue #453 for background context.

[amydevs]

From what I understand, this is a similar problem that React anonymous components have: https://julesblom.com/writing/component-displayname

The solution for dealing with it there is just to hardcode the fn component displayName, no other way.

[amydevs]

I've found one last place that the function.name is used in Discovery.ts:

public readonly discoverVertexHandlerId =
    `${this.constructor.name}.${this.discoverVertexHandler.name}.discoverVertexHandlerId` as TaskHandlerId;

Just to clarify, is this meant to be just ${this.constructor.name}.discoverVertexHandler or ${this.constructor.name}.discoverVertexHandler.discoverVertexHandlerId?

Because the naming convention for all of these TaskHandlerIds seem to be ${this.constructor.name}.${function_name}

[tegefaulkes]

It used to be ${this.constructor.name}.${this.discoverVertexHandler.name} but was changed to ${this.constructor.name}.${this.discoverVertexHandler.name}.discoverVertexHandlerId as a stop gap measure to fix a bug where the .names were undefined resulting in conflicting handlerIds.

It should just be the constructorName.handlername now.

[amydevs]

THis has been fixed by 80ac79d