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

deprecation(log): deprecate LogRecord class and use plain object instead #4390

Open
timreichen opened this issue Feb 25, 2024 · 5 comments
Open
Labels
feedback welcome We want community's feedback on this issue or PR

Comments

@timreichen
Copy link
Contributor

Is your feature request related to a problem? Please describe.

LogRecord is a class with only properties and getters. It seems the same can be achieved with plain js object.

Describe the solution you'd like

Deprecate the LogRecord class and export LogRecord an interface instead. Replace all new LogRecord() occurrences with a plain object.

export interface LogRecord {
  msg: string;
  args: unknown[];
  datetime: Date;
  level: number;
  levelName: string;
  loggerName: string;
}

Describe alternatives you've considered

Leave as is.

@iuioiua
Copy link
Collaborator

iuioiua commented Mar 3, 2024

I'm for this. Having a plainer/simpler symbol that achieves the same functionality is a good idea.

@iuioiua iuioiua added the feedback welcome We want community's feedback on this issue or PR label Mar 3, 2024
@timreichen
Copy link
Contributor Author

I just wonder how to introduce this without a breaking change. Should we just deprecate the class and introduce the interface after the class removal?

@iuioiua
Copy link
Collaborator

iuioiua commented Mar 11, 2024

I'm not sure there is a way to make a non-breaking change either. However, the change seems easy-to-make, from the user's perspective, and is a small dump in design simplicity. Any thoughts, @kt3k?

Before:

...
fileHandler.handle(
  new LogRecord({
    msg: "AAA",
    args: [],
    level: LogLevels.ERROR,
    loggerName: "default",
  }),
);
...

After:

...
fileHandler.handle({
  msg: "AAA",
  args: [],
  level: LogLevels.ERROR,
  loggerName: "default",
});

@iuioiua
Copy link
Collaborator

iuioiua commented Apr 26, 2024

@kt3k, would you be in favour of doing this in v1?

@kt3k
Copy link
Member

kt3k commented Apr 26, 2024

I don't understand the motivation. It doesn't seem solving any problem.

the same can be achieved with plain js object

I think this assumption is not strictly correct. The getter-only fields are currently immutable, but if we change it to plain object, they become mutable (readonly works for TypeScript, but it doesn't work for JavaScript).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback welcome We want community's feedback on this issue or PR
Projects
None yet
Development

No branches or pull requests

3 participants