Skip to content

Support Tracker Programs as output and support writing in different DEs #3

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

Merged
merged 11 commits into from
Feb 27, 2024

Conversation

anagperal
Copy link
Contributor

@anagperal anagperal commented Feb 14, 2024

📌 References

📝 Implementation

  • Change index instead of logger class now it's a function that initiates the logger and returns it
  • Create different classes for each type of logger
  • Add D2 repositories needed to support Tracker program as output
  • Add new script for testing tracker program and adapt others
  • Update README
  • Allow to log an array of Data Elements in tracker program

📹 Screenshots/Screen capture

Screencast.from.14-02-24.08.44.28.webm

🔥 Notes to the tester

  • Using script to just testing the logger (add corresponding url, auth and the ids needed. message is the Data Element Id of a Data Value of the event to be logged):
    yarn script-tracker-program-example --url "" --auth admin:district --program "" --tracked-entity "" --program-stage "" --enrollment "" --message "" --debug

  • Using yarn link to test logger in your app:

$ nvm use # Selects node in .nvmrc
$ yarn install
$ yarn build
$ cd build
$ yarn link

On another app:

$ yarn link "@eyeseetea/d2-logger"

@anagperal anagperal self-assigned this Feb 14, 2024
@anagperal anagperal marked this pull request as ready for review February 14, 2024 08:41
@anagperal anagperal changed the base branch from master to development February 14, 2024 08:48
@anagperal anagperal marked this pull request as draft February 14, 2024 09:05
@anagperal anagperal marked this pull request as ready for review February 14, 2024 09:29
@anagperal anagperal force-pushed the feature/create-output-tracker-program branch from c5f5d76 to 525055e Compare February 14, 2024 10:15
Copy link

@xurxodev xurxodev left a comment

Choose a reason for hiding this comment

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

thanks @anagperal

From the architecture point of view, it is OK for me.

The unique thing pending is to avoid the use of as Promise<Logger>

I think that I have found a solution changing the signature of initLogger:

export async function initLogger(config: LoggerConfig): Promise<Logger<string>| Logger<TrackerProgramContent>> {
    switch (config.type) {
        case "program":
            return ProgramLogger.create(config);
        case "trackerProgram":
            return TrackerProgramLogger.create(config);
        case "console":
        default:
            return ConsoleLogger.create();
    }
}

With this solution, is not necessary to pass generic from the outside of the library.

Wait to @tokland revision to receive the ok for this change

@anagperal
Copy link
Contributor Author

anagperal commented Feb 14, 2024

thanks @anagperal

From the architecture point of view, it is OK for me.

The unique thing pending is to avoid the use of as Promise

I think that I have found a solution changing the signature of initLogger:

export async function initLogger(config: LoggerConfig): Promise<Logger<string>| Logger<TrackerProgramContent>> {
    switch (config.type) {
        case "program":
            return ProgramLogger.create(config);
        case "trackerProgram":
            return TrackerProgramLogger.create(config);
        case "console":
        default:
            return ConsoleLogger.create();
    }
}

With this solution, is not necessary to pass generic from the outside of the library.

Wait to @tokland revision to receive the ok for this change

thanks @xurxodev ! I have tested this in the example scripts and it is giving me the following errors in logger.info and in logger.success (CC @tokland):

  • in trackerProgramLogger.ts:
Argument of type '{ config: { trackedEntityId: string; programStageId: string; enrollmentId: string; }; messages: { id: string; value: string; }[]; }' is not assignable to parameter of type 'string & TrackerProgramContent'.
  Type '{ config: { trackedEntityId: string; programStageId: string; enrollmentId: string; }; messages: { id: string; value: string; }[]; }' is not assignable to type 'string'
  • and in consoleLogger.ts

Argument of type 'string' is not assignable to parameter of type 'string & TrackerProgramContent'. Type 'string' is not assignable to type 'TrackerProgramContent'

@tokland
Copy link
Contributor

tokland commented Feb 14, 2024

I'll check it tomorrow.

@tokland
Copy link
Contributor

tokland commented Feb 15, 2024

Ok, I see what's going on. TS is right about the problem, Promise<Logger<T>> has a generic T and each branch returns a different logger, the T in return type cannot be possibly matched.

In any case, we need a different approach. For TS to match a generic type argument, it has to be used in the function argument. Also, it's not idiomatic to pass explicit type parameters (unless parameter-less wrappers), TS should be able to infer it. Proposal:

type LoggerType = {
    program: ProgramLogger;
    trackerProgram: TrackerProgramLogger;
    console: ConsoleLogger;
};

export async function initLogger<Config extends LoggerConfig>(
    config: Config
): Promise<LoggerType[Config["type"]]> {
    type Res = Promise<LoggerType[Config["type"]]>;

    switch (config.type) {
        case "program":
            return ProgramLogger.create(config) as Res;
        case "trackerProgram":
            return TrackerProgramLogger.create(config) as Res;
        case "console":
            return ConsoleLogger.create() as Res;
    }
}

And now there is no need to pass the type parameter, as it's inferred, 100% type-safe:

const logger = await initLogger({ type: "console" }); // -> ConsoleLogger

Note that we still need to do the as Res casting, but now it's a different case, here TS is not able to relate Promise<LoggerType[T["type"]]> with the narrowed config. This is a complex issue that is still open, more details here. But these as are not nothing to worry about, the casting is type-safe.

@anagperal
Copy link
Contributor Author

thanks @tokland for the solution and the explanation of the problem, it works perfectly! 🎉

@anagperal anagperal requested a review from xurxodev February 15, 2024 11:09
Copy link

@xurxodev xurxodev left a comment

Choose a reason for hiding this comment

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

thanks @anagperal

Copy link
Contributor

@Ramon-Jimenez Ramon-Jimenez left a comment

Choose a reason for hiding this comment

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

Thx @anagperal !

@Ramon-Jimenez Ramon-Jimenez merged commit 9a5258d into development Feb 27, 2024
@Ramon-Jimenez Ramon-Jimenez deleted the feature/create-output-tracker-program branch February 27, 2024 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants