Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
samholmes
force-pushed
the
sam/logbox
branch
from
34ece8c to
57f1ad7
Compare
swansontec
left a comment
swansontec
left a comment
There was a problem hiding this comment.
The lint cleanups in app.js need to go a bit deeper. I haven't reviewed the second commit, since I'm not the assigned reviewer.
| const nativePerformanceNow = (): number => { | ||
| // @ts-expect-error - this is a hack to get around the fact that window is not typed | ||
| return window.performance.now() |
There was a problem hiding this comment.
Take all this performance stuff, and move it to its own file. Do not attach it to the global scope, but instead export const pnow = and so forth for each performance function. If we want to do performance analysis in the future, we can just import { pstart, pend, ... } from '../../perf.ts' or whatever you call the new file, rather than grabbing it from the global scope.
There was a problem hiding this comment.
The reason why this is in global scope was for the convenience in putting it in any repo like login-ui. Admittedly since most of our code is in the webview, this doesn't help much although it would be nice to have some equivalent in there as well.
| console.log('***********************') | ||
|
|
||
| // @ts-expect-error | ||
| // @ts-expect-error - this needs an explanation |
There was a problem hiding this comment.
Indeed, it does need an explanation.
This is needed for our logging integration. When we replace console.log with our own custom logging, we use global.clog to keep forwarding logs to the normal place.
This is dumb. Instead, our logging infrastructure should use named variable like export const realLog = console.log, and then call realLog() in the logging system. That avoids polluting the global scope, and makes it clear where this is being used (TypeScript becomes aware of what's going on).
| setInterval(() => { | ||
| intervalCallback().catch((err: unknown) => { | ||
| console.error(err) | ||
| }) |
There was a problem hiding this comment.
Optional: This should really be makePeriodicTask, since that avoids overlapping work in cases where the async function takes more that 3s.
3 participants
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have: