Skip to content

refactor: improve logFn types #372

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kricsleo
Copy link
Member

resolves #351

Since logObj.date must be of type Date, we should enforce stricter parameter types to prevent runtime errors.
In addition to formatDate requiring the Date type, we also use the Date internally:

consola/src/consola.ts

Lines 418 to 422 in 5ac9ed7

const diffTime =
this._lastLog.time && logObj.date
? logObj.date.getTime() - this._lastLog.time.getTime()
: 0;
this._lastLog.time = logObj.date;

@kricsleo kricsleo marked this pull request as ready for review March 25, 2025 09:20
Copy link

codecov bot commented Mar 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 29.30%. Comparing base (24c98ce) to head (e9268d0).
Report is 57 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #372       +/-   ##
===========================================
- Coverage   61.08%   29.30%   -31.78%     
===========================================
  Files          13       43       +30     
  Lines        1380     1723      +343     
  Branches       58       87       +29     
===========================================
- Hits          843      505      -338     
- Misses        537     1192      +655     
- Partials        0       26       +26     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pi0
Copy link
Member

pi0 commented Apr 3, 2025

Thanks for PR but it seems type only getting stricter we need to fix runtime support somehow to not wrongly detect { date } as logobj.

@kricsleo
Copy link
Member Author

kricsleo commented Apr 3, 2025

Do you mean a stricter isLogObj function check, requiring date to be Date if it exists?

consola/src/utils/log.ts

Lines 16 to 33 in 5ac9ed7

export function isLogObj(arg: any) {
// Should be plain object
if (!isPlainObject(arg)) {
return false;
}
// Should contains either 'message' or 'args' field
if (!arg.message && !arg.args) {
return false;
}
// Handle non-standard error objects
if (arg.stack) {
return false;
}
return true;
}

@pi0
Copy link
Member

pi0 commented Apr 3, 2025

Yes exactly! (types couldbe lean as is IMO. feel free to make another PR for runtime fix)

@kricsleo
Copy link
Member Author

kricsleo commented Apr 3, 2025

#379 makes the runtime check more strict.

Personally, I prefer stricter types that align with runtime behavior. This can improve DX 😃

(I've also simplified the LogParam<T> for better understanding and easier maintenance)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants