-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Move console to console.ts and better stringify #141
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey thanks. I'm generally in favor of breaking the console code into a separate file. But there should be two commits - one moving existing code over and another adding the non-circular stuff. I've made some comments below.
console.ts
Outdated
|
||
const _print = V8Worker2.print; | ||
|
||
export class DConsole { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does D stand for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it to suppress a warning of my editor. Should have removed it, will do right away
console.ts
Outdated
return out.join(" "); | ||
} | ||
|
||
const _print = V8Worker2.print; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underscore is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above (seems to be considered as print in browser)
console.ts
Outdated
|
||
debug = this.log; | ||
info = this.log; | ||
dirxml = this.log; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's dirxml?
console.ts
Outdated
@@ -0,0 +1,82 @@ | |||
class ConsoleContext { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not
type ConsoleContext = Set<any>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite sure if there would be any more fields added to the ConsoleContext, so I left it that way so could later introduce new props if necessary. Could change to type though
72e7dad
to
846fd2b
Compare
Thanks. Completed splitting the original commits to multiple, removing console.dirxml, changing ConsoleContext. |
547f710
to
70457c2
Compare
Improved log format of functions, symbols, and class instances. |
4ff0521
to
05a8fd1
Compare
05a8fd1
to
a180acb
Compare
@ry Noticed that this PR has been held for quite a while. I can probably close it if it is not important for now |
@kevinkassimo Will get to it tomorrow - sorry for the delay! |
@kevinkassimo Thank you! It's a real improvement to the code base. And apologies again for slow response. |
Moving console definition to own file
console.ts
and make it a class.Replaced
JSON.stringify()
with customstringify()
to avoid crashing on circular objects and work with methods/functions by displaying[Function]
.Add some aliases to already implemented console methods.