refactor(gatsby-cli): rework cli to use ink #13089
Conversation
| const { parentSpan } = activityArgs | ||
| const spanArgs = parentSpan ? { childOf: parentSpan } : {} | ||
| const span = tracer.startSpan(name, spanArgs) | ||
| console.log = reporter.log |
There was a problem hiding this comment.
We should make sure we want to overwrite globals like this. It does solve the problem nicely re: plugins opting in to our reporter, though 🤔
There was a problem hiding this comment.
I'm not the biggest fan of this either but INK doesn't play super nicely with other process writing to stdout.
I've looked into the node codebase and they are actually just using util.format so I'm using it too.
https://github.com/gatsbyjs/gatsby/pull/13089/files#diff-5753f39bea69d2159152a45abdca82eaR87
I probably want to add some tests (maybe react snapshots could work here) to see if everything is wired correctly.
There was a problem hiding this comment.
yeah, my thinking is that instead of directly replacing console.log with reporter.log we should util.format what's passed in and then send it to reporter.log — we're going to overload the reporters so that core & plugins can send in structured data which can then be rendered by a component for richer reporting. So we can replace all the places in core where we're trying to do more sophisticated layout of messages with sending data to a component.
There was a problem hiding this comment.
we'll centralize design in some standard components which we can continue to iterate on over time + we'll have structured data that a) we can send out as JSON for --json output and that we can send to other UIs e.g. the coming Gatsby UI
There was a problem hiding this comment.
I think if we look at structured logging so have multiple ways of saving these logs I think it's better to move to winston and have an ink reporter as a plugin. (probably v3)
i'll like the console.log switch, let me update that.
|
@KyleAMathews I added Gifs and links to asciinema |
|
Note to myself need to check our sources to see what api yurnalist provides and which we use. for example |
| _addMessage(type, str) { | ||
| // threat null/undefind as an empty character, it seems like ink can't handle empty str | ||
| if (!str) { | ||
| str = `\u2800` |
There was a problem hiding this comment.
I think this issue is fixed now, you can use empty string here ;)
|
Amazing, excited to see this PR ship! 🎈 |
|
Let me know if you run into any issues with Ink. |
|
@wardpeet once the e2e test is fixed (which required upstream changes?) is this ready to go? If so -- 🎉 |
wardpeet
added
status: awaiting reviewer response
|
yurnalist is used on CI to make sure we pipe to stderr as ink does not which means azure's OOM is fixed 😛 |
There was a problem hiding this comment.
I'm not 100% sure about the reporter design. It feels we'll have to extract lots of that stuff from component when we move to structured logging. It also feels that the thing we are actually replacing isn't reporter, but renderer for the docs. Ultimately, there will be only structured log reporter and we'll have one of the renderers (with options like color disabling) available. Printing JSON would be one of the renderer, ink another and so on. However I feel that I shouldn't bikeshed too much around that, so this is fine as it is.
I don't really understand this comment. What do you mean with extracts lots of that stuff? For structured logging, a message can be an object and the logic of displaying or hiding a message is still part of the reporter. |
|
My comment is just about general architecture of this. Let's move it out of the context of this PR, I think it's fine as it is otherwise. |
|
|
Replaces our augmented yurnalist reporter with ink. Everything still works the same as before so no changes were made to the reporter api.
Description
This PR brings the world of CLI & React to Gatsby with
This PR replaces our augmented
yurnalistreporter with ink. Everything still works the same as before so no changes were made to the reporter api.One extra thing that has been done is mapping the native console API into the reporter as well so everything is handled by INK.
Gatsby build:
Gatsby develop:
Related Issues
Partially fixes #11076
cc @vadimdemedes @sindresorhus if they like to give some pointers