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.

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

Implement console.Console constructor. #3625

Closed
DavidIsa opened this issue Jul 12, 2023 · 14 comments · Fixed by #5448
Closed

Implement console.Console constructor. #3625

DavidIsa opened this issue Jul 12, 2023 · 14 comments · Fixed by #5448
Assignees
Labels
atw bug Something isn't working good first issue Something that would be good for new contributors node.js Compatibility with Node.js APIs

Comments

@DavidIsa
Copy link

What version of Bun is running?

0.6.13

What platform is your computer?

Linux 5.15.49-linuxkit-pr aarch64 aarch64

What steps can reproduce the bug?

Execute this code in bun

const log = new console.Console();

What is the expected behavior?

Should not fail to call new console.Console() and must create a new Console instances.

What do you see instead?

const log = new console.Console();
                ^
TypeError: undefined is not a constructor (evaluating 'new console.Console')

Additional information

This method is implemented in nodejs and some npm packages use it which causes it to fail when using it with bun.

Method documentation for nodejs https://nodejs.org/api/console.html#new-consoleoptions

@DavidIsa DavidIsa added the bug Something isn't working label Jul 12, 2023
@paperclover paperclover added the good first issue Something that would be good for new contributors label Jul 13, 2023
@adsellor
Copy link

If you don't mind, I would like to work on this issue.
cc: @paperdave

@Electroid Electroid added the node.js Compatibility with Node.js APIs label Jul 13, 2023
@paperclover
Copy link
Member

go for it

i would add this in ConsoleObject.ts

basically implement it in js using Bun.inspect

make it similar to how the async iterator works in terms of wiring it up to the console object.

@adsellor
Copy link

Great. Thanks for the tips. I'll get started on this

@joaogl
Copy link

joaogl commented Jul 27, 2023

Hey @adsellor, I'm also facing this issue while trying to start my nestjs project.
How's the progress on this?
is there anyway I can help?

@adsellor
Copy link

adsellor commented Jul 27, 2023

Hey @joaogl. Sorry for taking this long and not updating my findings here. Here is what I found while trying to make this work.

  • In NodeJS the Console constructor accepts either (stdout, stderr) parameters, or a single options parameter.
  • The console itself is an instance of Console constructor where stdout and stderr are passed are set to process.stdout and process.stderr.
  • In Bunjs, there is (ZigConsoleClient)[https://github.com/oven-sh/bun/blob/main/src/bun.js/bindings/exports.zig#L921], which is initialized (here)[https://github.com/oven-sh/bun/blob/main/src/bun.js/javascript.zig#L937] and assigned to the global VM.

I wasn't able to do it ConsoleObject.ts, as @paperdave suggested, as it requires initialization of ZigConsoleClient itself (Maybe I just missed something and it can be done).
But essentially, what I was trying to do, is assigning Console to the ZigConsoleClient and returning a new instance, and extending the client itself to support all the options that Node does (like inspectOptions and colorMode). However, we can't pass Writer as a parameter with callconv(.C). I've also tried implementing it in ZigConsoleClient.cpp, but I don't think that extending it with a non-JSC default is a good idea.
A lot of things got into my way last week and I didn't really have time to explore other options, so if you are down to help, your input would be much appreciated.

@joaogl
Copy link

joaogl commented Jul 27, 2023

Hey @adsellor, don't expect too much from me :D first time looking into a codebase like this one.

Is there any documentation around the codebase structure or architecture besides what is in https://bun.sh/docs/project/development that I could use to warm up?

@adsellor
Copy link

Hey @adsellor, don't expect too much from me :D first time looking into a codebase like this one.

Is there any documentation around the codebase structure or architecture besides what is in https://bun.sh/docs/project/development that I could use to warm up?

I don't think so, and I don't feel confident enough to give an advice on where to start :D . But maybe join discord and ask questions along the way.

P.S I'll get back to this during this weekend, hoping to have at least a simple Console constructor with stdout working.

@paperclover
Copy link
Member

paperclover commented Jul 28, 2023

Is there any documentation around the codebase structure or architecture besides what is in https://bun.sh/docs/project/development that I could use to warm up?

unfortunatly there isnt. but if you're in our discord i can answer any questions about the project. in #dev-general there.

this documentation page really needs updating.

@paperclover
Copy link
Member

also, the idea i was thinking was to use Bun.inspect* or util.inspect to format the arguments to a string, then write the string to the given node stream.

I dont think we need to do much of this in Zig/C++. I don't really imagine this feature is used too heavily that it needs to be extremely fast either.

Also it's stdout and stderr, not stdin. I was confused for a second before checking docs.

*the note about Bun.inspect is right now it actually does almost exactly what we need, but we want to change the signature of this function from inspect(...): string to inspect(value, options): string to be a little closer to node's util inspect (and be more useful overall). But in this case it's probably worth keeping the version of inspect that works like a console formatter, maybe we'll do that only as a builtin function.

@adsellor
Copy link

Also it's stdout and stderr, not stdin. I was confused for a second before checking docs.

Sorry for the confusion, it should've been stderr instead of stdin, my mind was somewhere else. I've updated the comment.

*the note about Bun.inspect is right now it actually does almost exactly what we need, but we want to change the signature of this function from inspect(...): string to inspect(value, options): string to be a little closer to node's util inspect (and be more useful overall). But in this case it's probably worth keeping the version of inspect that works like a console formatter, maybe we'll do that only as a builtin function.

I see, I'll try to implement it this way. Thanks for the advice.

@joaogl
Copy link

joaogl commented Jul 28, 2023

@adsellor if you don't mind, please share the PR once you start looking into this, I'm deeply interested in understand how this will turn out and to look into the changes you make to learn something 🙏

@webivan1

This comment was marked as outdated.

@Electroid
Copy link
Contributor

@webivan1 Thanks for sharing, that might work for your particular script, but we'll still need to implement console.Console.

@webivan1
Copy link

@Electroid Sorry, I forgot to mention It was for nestjs framework
I had same error console.Console when I use docker image oven/bun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
atw bug Something isn't working good first issue Something that would be good for new contributors node.js Compatibility with Node.js APIs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants