Skip to content

Add server logs#8

Merged
localjo merged 5 commits into
masterfrom
add-server-logs
Jul 2, 2014
Merged

Add server logs#8
localjo merged 5 commits into
masterfrom
add-server-logs

Conversation

@localjo
Copy link
Copy Markdown
Member

@localjo localjo commented Jul 1, 2014

This pull request adds server logs to the devtools. Once it is merged it will resolve #1

@localjo
Copy link
Copy Markdown
Member Author

localjo commented Jul 1, 2014

@pushred This commit adds the server logs to the panel via socket.io, so they'll stream in as new messages are logged. Styling will be improved when I attack issue #2 but there may also be some room for improvement in the future with the format of the logs (right now they're just strings). Future log enhancements might call for some additional CSS or syntax highlighting or something.

@joanniclaborde you probably know best what would be feasible/useful to output in the logs in addition to what's there now.

@localjo localjo added this to the v1.0.0 milestone Jul 1, 2014
@localjo localjo self-assigned this Jul 1, 2014
@pushred
Copy link
Copy Markdown
Member

pushred commented Jul 2, 2014

Note that I've rebased https://github.com/solidusjs/solidus/tree/stream-log-to-web-socket to pull in the new Solidus header.

No comments on the code front, this is all super straightforward. Works as advertised!

I've definitely got a lot of ideas on ways to present the logs (special handling around exceptions, slowness, resource fetching) but I think it's all going to require structured data to be provided vs. the simple strings we have now. Switching to a logging library that'll facilitate that is definitely the plan but Joannic will be focusing on another project for much of this month. So this is totally fine for now. Perhaps if anything you can color code the log entries a bit with some simple string matching.

You'll definitely need to prioritize displaying the logs in a scrollable panel that keeps the scroll position at the bottom, like the console, terminal, etc. But as you noted #2 will take care of that.

Another detail would be standardizing any feedback concerning the Solidus server connection, i.e. "Looks like you're not inspecting a Solidus Page", "Socket Disconnected", etc. I'd present those big & centered in the panel. Messaging could be better as well. The first message isn't bad though I'd consider removing the tab altogether if possible in this scenario. The latter something like "Your Solidus dev server doesn't seem to be running anymore" would be good.

We'll also need to figure out how to communicate the Socket.io server to you, I think another header would work for that. If it isn't present you'd disable this functionality as that would signal either an older Solidus server or something running in a production environment which should definitely not be running the server. Perhaps there might also be some value in adding a header with the current environment to communicate that (and explain missing features).

localjo added a commit that referenced this pull request Jul 2, 2014
@localjo localjo merged commit 0375c13 into master Jul 2, 2014
@localjo localjo deleted the add-server-logs branch July 17, 2014 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Server Error Logs

2 participants