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

Feature Request: Make customization practical/usable #241

Open
dever23b opened this issue Jul 1, 2023 · 2 comments
Open

Feature Request: Make customization practical/usable #241

dever23b opened this issue Jul 1, 2023 · 2 comments
Labels
enhancement New feature or request

Comments

@dever23b
Copy link

dever23b commented Jul 1, 2023

Description / Use Case for a Feature

The new version of TSLog boasts the ability to "overwrite pretty much every aspect of the log processing"; however, while technically true, it isn't practical to use. Please work on the usability of the feature.

In order to overwrite a part of the lifecycle, even for a very simple tweak, and then return to normal workflow, we have to recreate substantial portions of internal code. So many of the internal methods are either designated private or not exported, so once we attempt to overwrite anything, there doesn't seem to be any practical way to rejoin the normal workflow without a massive out-of-scope rewrite effort. The way the methods are presented in the package (whether explicitly private or hidden via the package.json), any simple overwrite seems to require an extremely in depth, recursive recreation of every dependent task that the feature being overwritten performs; this makes the overwrite feature just not worth the effort.

There should be a way to link back into default behavior and processing once we've accomplished whatever tweaks we needed with the overwrite.

Example;
If I want to overwrite toLogObj, to enhance it in any way, the code in BaseLogger.log no longer calls its internal _toLogObj method -- which is reasonable: this is what I'm overwriting. In my case, I was hoping to manipulate the file paths and convert any parenthesis to something else, to make a workaround for the bug #240. This could be a trivial task; however, I can't do it without going down a very deep rabbit hole of recreating code that has nothing to do with my very simple mod. The problem is that I'm now prevented from linking back into any default behavior. _toLogObj uses: isError, _toErrorObject, and isBuffer. One of those methods is private; the other two are not accessible due to how the package.json exports are configured. This problem is recursive. Therefore, if I overwrite _toLogObj, I am now forced to also rewrite every single internal dependency that extends from that method. The task almost immediately spins out of control, becoming a massive out of scope undertaking.

@terehov
Copy link
Contributor

terehov commented Jul 2, 2023

Thank you for your detailed description. I see your point and am happy to accept a MR.
I wanted to keep a clean API, but what is your suggestion, which methods would you want to expose?

@BenceSzalai
Copy link

Hi! I am facing the same issue. I would like to customise some aspects while relying on the original behaviour for whatever stays the same. It is almost impossible without rolling a completely own logging solution keeping only the skeleton of tslog or forking it an do customisation directly there.

Private methods

As per the issue, on one hand is there any reason why so many methods on BaseLogger are private? When someone implementing the library wants to override behaviour it would be very good to still be able to call the original implementation, and only modify the input data or the results. That way (sticking to the example above) overriding _toLogObj could be this simple:

export class MyCustomisedLogger<LogObj> extends BaseLogger<LogObj> {
  public _toLogObj(args: unknown[], clonedLogObj: LogObj = {} as LogObj): LogObj {
    // Do some things to `args` and `clonedLogObj`
    const logObj = super._toLogObj(args, clonedLogObj)
    // Do some more things to `logObj`
    return logObj
  }
}

It is not clear why it is not allowed by marking those methods private. Imho all those internal methods could be made public, this would jumpstart versatility in customising behaviour.

Masked exports

The other part is that many of the utilities used by the package are masked and cannot be accessed from outside. This - as described above - makes replacing certain behaviours a huge burden, because not only the parts we want to change need to be rewritten, but the whole feature. This drives the developers to copy-paste code from the package to their own codebase, which is very suboptimal (e.g. any issues fixed in the package now manually need to be ported to the copied code, which requires monitoring).

Again, it is not clear why those methods are hidden from the external world if on purpose? But in an ideal case they would be exposed and reusable. One solution could be to add another export for them, such as:

{
  "name": "tslog",
  "exports": {
    ".": {
      "types": "./dist/types/index.d.ts",
      "require": "./dist/cjs/index.js",
      "import": "./dist/esm/index.js",
      "default": "./dist/esm/index.js"
    }
    "./deps/*": {
      "types": "./dist/types/*/*.ts",
      "require": "./dist/cjs/*/*.js",
      "import": "./dist/esm/*/*.js",
      "default": "./dist/esm/*/*.js"
    }
  },
}

With that in place one could simply reuse the parts in the original issue, e.g.:

import { isError, isBuffer } from 'tslog/deps/runtime/nodejs/index.js'

(Or at least I think, haven't tried, the export/import path mappings might need some tweaking, but I think the general idea is clear. Also I'm not sure how this would work on the browser side...?)

Internal vs. Public API

Okay, fair enough, both the utilities in the additional files and the methods in BaseLogger are part of the internal API. Generally it creates a burden for any library to expose internals, because if public perceives it as part of the public package API, the package maintainer in a sense looses the freedom to change them anytime.

But on the other hand there is the argument of this issue. So in my world there should be a way to say: relying on any of those internals is something to do at your own risk and they are not guaranteed to stay the way they are. Still anyone can pin the version they are using if that is a problem for them. As per myself, I'd rather adjust my customisations every now and then when code breaks due to updates changing methods that are not part of the official public API as opposed to having to copy and paste / rewrite huge chunks of (by the way great) generic behaviour code.

Because at that point I'd rather just fork the package and do the changes I need in there. The maintenance burden would already be totally with me, so it would still be much cleaner to maintain a repo and merge upstream changes every now and than.

Which is fine tbh, sometimes we need to fork things. The only point here is that the library promises great customisability. The only question is what is left from the original implementation once we start to customise the way it is currently possible. Looks like not much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants