improve extensibility of logtape

[okikio]

Improve the extensibility of logtape.

1. Currently logtape has a number of exported functions that are are useful and necessary to build on top of logtape but aren't accessible, this pr adds all the missing modules to package exports for both dnt and jsr, this should make it easier to build on top of logtape.
2. The logTemplate and logLazily methods are limited in odd ways compared to the log method which makes it difficult to extend logtape and create custom log functions, e.g. group, groupCollapsed, dir, dirxml, timeStart, timeEnd, table, etc... by adding the properties parameter to both methods we can now store a groupId in properties consistently, or maybe even store that the expected output is a table, etc...
3. The addition of the bypassSink parameter is for just in case situations, if the log method can use bypassSink I am not sure why logTemplate and logLazily shouldn't be able to do the same, I can always remove the parameter if people feel it isn't a good fit.

[codecov]

Codecov Report

Attention: Patch coverage is 40.00000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 93.94%. Comparing base (efce6f3) to head (0cdf993).

Files	Patch %	Lines
logtape/logger.ts	40.00%	18 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main       #6      +/-   ##
==========================================
- Coverage   96.23%   93.94%   -2.30%     
==========================================
  Files           9        9              
  Lines         717      743      +26     
  Branches      132      134       +2     
==========================================
+ Hits          690      698       +8     
- Misses         26       44      +18     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dahlia]

Thanks for your first contribution! I left few comments! Plus, could you add entries to the changelog?

[dahlia]

The current exports are very carefully curated by me. The APIs that are not exported are not exposed for a reason. Are there any APIs that you would like to see exposed?

[okikio]

For some background, I'm basically trying to wrap the logtape loggers in a abstract logger class, the goal is basically to be able to register the various loggers that someone wants to use, and to be able to use them as such,

https://gist.github.com/okikio/dab52147d880773a187f48de2b2da689

// Interface to extend Logger with custom loggers
export type LoggerWithCustomLoggers<L extends string> = Logger<L> & {
  [P in L]: Logger<L>;
}

// Function to create a logger with typed custom loggers
export function createLogger<L extends string>(options: LoggerOptions<L> = {}): LoggerWithCustomLoggers<L> {
  return new Logger(options) as LoggerWithCustomLoggers<L>;
}

export const initLogger = getLogger(["@bundle/core", "init"]);
export const buildLogger = getLogger(["@bundle/core", "build"]);
export const generalLogger = getLogger(["@bundle/core", "general"]);

const logger = createLogger({
  loggers: {
    init: initLogger,
    build: buildLogger,
    customLogger: getLogger(["@bundle/core", "custom"]),
  },
});

// Logs to the general logger by default
logger.info`General Log ${5}`;

// Logs to the init logger with a custom template
logger.with({ logger: "init", message: "start {time}" }).warn({ time: Date.now() });

// Logs an error to the init logger
logger.with({ logger: "init" }).error("error {message}", { message: "Error message" });

// Logs an error to the init logger (with context)
logger.with({ logger: "init" }).context({ message: "Error message" }).error("error {message}");

// Logs an error to the init logger (with context no. 2)
logger.with({ logger: "init", context: { message: "Error message" } }).error("error {message}");

// Shortcut for init logger
logger.init.error("Initialization failed: {error}", { error: new Error("cool") });

// Logs to the custom logger
logger.customLogger.info("Custom logger in action");

// Shortcut for build logger
logger.build.fatal`A fatally cool message`;

// This will now cause a TypeScript error, as 'nonExistentLogger' is not registered
// logger.nonExistentLogger.info("This should not compile");

To achieve this workflow I need to add context to the logger, but that get's complicated as the original logtape Logger class doesn't really support the concept of contextual logs (though it is possible to replicate).

In addition, logtape has a couple of unhandled edge cases that context can help with, e.g. what happens if the user wants to log out a variable that ends up being a function, etc... For this to work I need to be able to fully control how log records are emitted, that's the main reason for needing all the modules to be exported in the exports field.

[okikio]

The API's I would need access to are basically all types and interfaces, the LoggerImpl class, the log, logLazily, logTemplate & emit methods, and parseMessageTemplate & renderMessage functions.

[dahlia]

Looking at your gist you linked to, I still don't understand how LoggerImpl and its methods are used. 🤔

Basically, I don't want to expose things like LoggerImpl because the wider the API surface, the harder it is to change the internal implementation without breaking backwards compatibility.

However, if you have a feature that you need in LogTape, it may be better to modify LogTape itself rather than extend it externally. I'm not sure what exactly the context you're trying to implement is for though. 😅

[okikio]

For LoggerImpl if possible exporting it as a type would potentially solve that issue. But I would still need access to the parseMessageTemplate & renderMessage functions. Not sure what can be done about that.

[okikio]

I've made a couple changes to the pr, I've created a new types.ts module where I just export all the types instead of exporting all the code, I then export parseMessageTemplate and renderMessage in the mod.ts file, what do you think?

[dahlia]

The current exports are very carefully curated by me. The APIs that are not exported are not exposed for a reason. Are there any APIs that you would like to see exposed?

[okikio]

I've updated the changelogs 👍🏽

[dahlia]

I left some comments.

I still don't quite understand the purpose of the feature called context that you want to implement, but I guess it's one thing to implement it directly into LogTape if you need to. What do you think of this direction?

[dahlia]

Why was this the only one aliased?

[okikio]

There is a type conflict with getFileSink in ./sink.ts and ./filesink.jsr.ts, I wasn't sure how to approach the conflict.

[dahlia]

According to Codecov, this code is not covered by tests, could you please add a test to cover this part?

[dahlia]

According to Codecov, this code is not covered by tests, could you please add a test to cover this part?

[dahlia]

Do you need all of these types? 😅

[okikio]

For extensibility purposes yes, all the types are required, however, if I'm building in context into logtape then the need decreases a bit, but other developers extending on logtape might still need them.

[okikio]

The main reason why I didn't is that I didn't want change the direction of project, but if you're up for I'd love to create a pr that adds support for context.

[dahlia]

@okikio I apologize for making you work multiple times, but if you don't mind, I'd like to close this PR, and could you open an issue for the concept of context instead? 😅 We can talk about the design a bit and then you can get to work on it, or I can work on it.

[okikio]

Sure, I'll close this pr in the meantime then

[okikio]

@dahlia I've opened a new issue #7, we can continue the conversation there.