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.

Sign up for GitHub

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

Jump to bottom

v2: First steps, drop old IE support, fix debug #203

Merged
merged 9 commits into from
Dec 23, 2024
Merged

v2: First steps, drop old IE support, fix debug #203

merged 9 commits into from
Dec 23, 2024

Conversation

Mr0grog
Copy link
Contributor

@Mr0grog Mr0grog commented Dec 19, 2024 •
edited
Loading

This is a very first step towards v2, based on discussion in #119. It just does two main things:

  1. Drop support for IE < 11 — lazy console, special trace handling, etc.
  2. Make debug call through to console.debug instead of console.log (fixes log.debug should map to console debug in Chrome #137).

This does not:

  • Change tooling or switch to ESM. Those should probably be the next PR.
  • Make any changes to persistence. I’m also leaving that to a separate PR.

⚠️ Unfortunately, there is one debatable bit here! We don’t currently have a level reserved for log, so I’ve left it as an alias for debug (it used to be the other way around). Should we add one? (I think yes.) Where does it sit in terms of levels? Precedent varies:

  • The same as info (Chrome)
  • Between debug and info (closest to Loglevel’s v1.x behavior)
  • Between info and warn (Safari & Firefox)

I’d vote for “between info and warn” to match browsers as well as possible.

Other options could include:

  • Dropping it since it isn’t exactly a “level” (i.e. not having a logger.log() method at all)
  • Making it a way to call with a dynamically chosen level (rather than a pass-through to console), i.e. logger.log(level, ...messages))
  • Making it have the same level as another method but still call console.log under the hood (right now it is the same level as debug, and also calls console.debug). If we did this, maybe best to make it match the info level (Since that is how Chrome treats it)?

(Update: re-formatted all the stuff about log() to make it more clear.)

pimterry and others added 6 commits December 19, 2024 12:23
@pimterry
Update master to main
4cdd146
@Mr0grog
Drop special old IE behavior
4710d8a 
As a first step for v2, this drops support for behaviors only needed with IE < 11:
- A `console` object that appears asynchronously only after dev tools are opened.
- Oddball `trace` behavior.
- Missing `<function>.bind()` method.
@Mr0grog
Merge realMethod into defaultMethodFactory
89c939d
@Mr0grog
Make debug actually call console.debug
f9d5678 
For now, this keeps `log` as an alias for `debug`, since we don't currently have a separate `log` level. We may want to add one!

Fixes pimterry#137.
@Mr0grog
Add README warning for v2
763fb72
@Mr0grog
Build!
0f0b265
@Mr0grog Mr0grog mentioned this pull request Dec 19, 2024
Open
@Ryuno-Ki
Copy link

Can you switch the target branch?
The README is claiming that this is going to be an in-development branch, but after merge, this is part of main which will therefore no longer be „stable”.

@Mr0grog Mr0grog changed the base branch from main to v2 December 20, 2024 14:07
@Mr0grog
Copy link
Contributor Author

Mr0grog commented Dec 20, 2024

Whoops, thought I'd done that! Thanks for catching.

@Mr0grog
Copy link
Contributor Author

Mr0grog commented Dec 21, 2024

Should we add [a log level]? (I think yes.) Where does it sit in terms of levels?

Looking back at this at the end of the day, I think I’ve changed my mind. The README has long made clear that the .log() method is just there to ease migration from console and is not really a “level”:

log.log(msg) is also available, as an alias for log.debug(msg), to improve compatibility with console, and make migration easier.

So actually the way this PR is now is probably fine.

The only change I might still suggest is making log an alias for info instead of debug in order to work more like browser dev tools:

  1. That is how Chrome treats it and
  2. Other browsers put it between info and warn, putting it closer to info than debug.

But that’s not a strong opinion. The current level .log() is aliased to does not seem to be a problem for anyone.

pimterry
pimterry approved these changes Dec 23, 2024
View reviewed changes
Copy link
Owner

@pimterry pimterry left a comment

Choose a reason for hiding this comment

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

The only change I might still suggest is making log an alias for info instead of debug in order to work more like browser dev tools

Yep, that makes sense for me! As in the README, this was intended just as a little convenience method, not as a separate level, and I think that matches normal levels elsewhere so I don't think we should make it an actual loglevel in itself. Aliasing to info seems reasonable though.

Happy to merge this now and leave it for a separate PR, or include it here if you prefer, up to you.

lib/loglevel.js Show resolved Hide resolved
dist/loglevel.min.js Show resolved Hide resolved
@Mr0grog
Alias log() to info() instead of debug()
9f1dcb5 
This matches browser behavior more closely (Chromium/Blink treats them the same; Webkit & Gecko put it between `info` and `warn`, so this is closer to their behavior than the old alias).
@Mr0grog Mr0grog force-pushed the version-two-step-one branch from b66c353 to 9f1dcb5 Compare December 23, 2024 19:00
@Mr0grog
Copy link
Contributor Author

Mr0grog commented Dec 23, 2024 •
edited
Loading

Aliasing to info seems reasonable though.

Happy to merge this now and leave it for a separate PR, or include it here if you prefer, up to you.

@pimterry I went ahead and made that change, so feel free to merge.

Re: the two issues you called out inline, please see my replies. Happy to make changes if you want.

@pimterry pimterry merged commit 1cfb7bd into pimterry:v2 Dec 23, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pimterry pimterry pimterry approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

log.debug should map to console debug in Chrome
3 participants
@Mr0grog @Ryuno-Ki @pimterry