refactor(timing/utils-body): a few minor edits #3557
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3557 +/- ##
==========================================
+ Coverage 94.27% 94.71% +0.44%
==========================================
Files 157 157
Lines 9501 9537 +36
Branches 2854 2783 -71
==========================================
+ Hits 8957 9033 +76
+ Misses 544 504 -40 ☔ View full report in Codecov by Sentry. |
|
Hi @sharma-shray, thanks for the suggestion. It may seem like a change that is obvious to you and doesn't need to be explained, but from the perspective of maintenance, it would be appreciated if the “why” was written in the commit message. Please use the following format as a reference for writing messages in future commits. |
src/jsx/streaming.ts
Outdated
| @@ -27,7 +27,7 @@ export const Suspense: FC<PropsWithChildren<{ fallback: any }>> = async ({ | |||
| fallback, | |||
| }) => { | |||
| if (!children) { | |||
| return fallback.toString() | |||
| return await fallback.toString() | |||
There was a problem hiding this comment.
Please add cases where this change is necessary to the test.
src/middleware/timing/timing.ts
Outdated
| @@ -30,7 +30,9 @@ interface TimingOptions { | |||
| const getTime = (): number => { | |||
| try { | |||
| return performance.now() | |||
| } catch {} | |||
| } catch (e) { | |||
| console.warn('performance.now() is not available. Falling back to Date.now()', e) | |||
There was a problem hiding this comment.
There are already several console.warn() in this file, but these are displayed when “middleware is not set up correctly”.
This fallback to Date.now() is displayed when running in an environment where performance.now() is not available, but I don't think users would be happy if they were shown a warning for every request.
If we display a warning when it is not available, it would be better to do it like this.
diff --git a/src/middleware/timing/timing.ts b/src/middleware/timing/timing.ts
index d387f43b..f341592f 100644
--- a/src/middleware/timing/timing.ts
+++ b/src/middleware/timing/timing.ts
@@ -27,12 +27,9 @@ interface TimingOptions {
crossOrigin?: boolean | string | ((c: Context) => boolean | string)
}
-const getTime = (): number => {
- try {
- return performance.now()
- } catch {}
- return Date.now()
-}
+const getTime: () => number =
+ performance?.now?.bind(performance) ??
+ (console.warn('performance.now is not supported, using Date.now'), Date.now.bind(Date))
/**
* Server-Timing Middleware for Hono.There was a problem hiding this comment.
Hello! thanks for you response
Now I really think that was unnecessary, was planning to add some outputs about possible bugs, but thanks for your comments!
There are already several
console.warn()in this file, but these are displayed when “middleware is not set up correctly”.This fallback to
Date.now()is displayed when running in an environment whereperformance.now()is not available, but I don't think users would be happy if they were shown a warning for every request.If we display a warning when it is not available, it would be better to do it like this.
diff --git a/src/middleware/timing/timing.ts b/src/middleware/timing/timing.ts index d387f43b..f341592f 100644 --- a/src/middleware/timing/timing.ts +++ b/src/middleware/timing/timing.ts @@ -27,12 +27,9 @@ interface TimingOptions { crossOrigin?: boolean | string | ((c: Context) => boolean | string) } -const getTime = (): number => { - try { - return performance.now() - } catch {} - return Date.now() -} +const getTime: () => number = + performance?.now?.bind(performance) ?? + (console.warn('performance.now is not supported, using Date.now'), Date.now.bind(Date)) /** * Server-Timing Middleware for Hono.
yusukebe
changed the title
|
Thank you for the PR. Looks good. But next time, please follow the @usualoma 's comment. |
* upd * upd * upd * upd * Add files via upload
The author should do the following, if applicable
bun run format:fix && bun run lint:fixto format the code