feat: add console hook to static template

[SSHari]

What kind of change does this pull request introduce?

Suggested implementation for #908.

What is the current behavior?

The static template doesn't capture logs from the iframe and forward them to the parent so they can be displayed in the Console component.

What is the new behavior?

The static template makes use of the console-feed library to capture iframe logs to be consumed by a parent Console component.

What steps did you take to test this? This is required before we can merge, make sure to test the flow you've updated.

I verified this in Storybook by adding a log statement (e.g. <script>console.log('Test');</script> to the head element of a static template example.

Checklist

• Documentation;
• Storybook (if applicable);
• Tests;
• Ready to be merged;

This is a very rough pass. I wanted to get some thoughts about the implementation before digging deeper. Some things that should be considered for a more complete solution:

1. Move the consoleHook file to a higher level so it can be used by both the node and static clients (i.e. don't duplicate the file and build step)
2. We should make sure that the event listener is properly cleaned up. I added a step to the destroy method to clean up the event listener, but I still need to verify that it will always be called when the static client is torn down
3. This implementation injects the consoleHook JS into the head of the document. It makes use of the injectContentIntoHead helper so that the code is hidden from the end user, but it's still a good amount of code to inject into every static template (especially if the console isn't even used). We may consider putting this behind a flag so that it doesn't always happen.

[vercel]

@SSHari is attempting to deploy a commit to the CodeSandbox Team on Vercel.

A member of the Team first needs to authorize it.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit f726fe0:

Sandbox	Source
Sandpack	Configuration
sandpack-run-stale-value	Configuration

[danilowoz]

Hey, @SSHari. Thanks for taking this issue!

Responding to your questions:

1. I think this is a good idea. Feel free to move the consoleHook file.
2. This is a good one! Thanks for fixing it.
3. I agree, so I was wondering if we can kind of "register" if the user is using any console listener.

[SSHari]

Hey, @SSHari. Thanks for taking this issue!

Responding to your questions:

1. I think this is a good idea. Feel free to move the consoleHook file.
2. This is a good one! Thanks for fixing it.
3. I agree, so I was wondering if we can kind of "register" if the user is using any console listener.

@danilowoz I like the idea of "registering" if the user is using any console listeners. From an implementation standpoint, this may be a little tricky.

Right now, the only thing that gets passed to a Client's listen method is a listener function. There's no way to really infer what message types the listener is expecting because it's an arbitrary function.

I think we could probably tweak this so that you can pass an optional array of message types to the listen method. We could use that array to track which types of messages are subscribed to and even conditionally call the listeners only if it's in that array.

Something like the following:

type MessageTypes = 'console' | 'start' | '...etc';
type ListenerOptions = { messageTypes?: Array<MessageTypes> };

function listen(listener: ListenerFunction, opts?: ListenerOptions) {
    opts?.messageTypes?.forEach(this.registerMessageType); // Track the message types

    return this.emitter.listener(message => {
       if (!opts?.messageTypes || opts.messageTypes.includes(message.type)) {
           listener(message);
       }
   });
}

If no array is passed then we'd just leave the functionality as is and just call the listener for every message.

That said, adding this property here means that we'd also need to properly update the addListener function in the useClient hook. An example function signature could be:

type AddListener = (
    listener: ListenerFunction,
    clientId?: string,
    opts?: ListenerOptions
) => UnsubscribeFunction;

It's a little awkward because if you wanted to set a global listener with the options, you'd have to do something like this:

addListener(listener, undefined, opts);

That said, there are definitely some potential problems with the above. Unless we require the list of messageTypes to be passed in, an end user can build their own implementation of the Console (i.e. with their own listeners) without passing in the messageTypes and it won't be immediately obvious why things aren't working.

I'm also not so sure that we can require it without that being considered a breaking change. If we did want to go this route, we could probably add deprecation warnings, but that may be a bit heavy-handed.

Anyways, I may be overthinking this completely. There might be a much simpler way to accomplish this. I'll give it some more thought on my end as well. When you get a chance, let me know what you think.

[SSHari]

Hey @danilowoz, have you given this any thought? I updated the PR with the consoleHook changes (i.e. move it to a higher location so it can be reused in both clients).

I also made the proposed changes above to track which message types are subscribed to. I'm still unsure if we want to go this route, but I thought getting a more fleshed-out example in front of others would be good. I made the changes in a separate commit, so it should be easy enough to back out if we don't want to move forward with this.

[danilowoz]

Thank you for implementing this; it looks great. However, I apologize for not being responsive here as we've been quite busy at CodeSandbox. Before merging such a "disruptive" change, I would like to understand how many people are interested in this feature.

How does this sound to you?

[SSHari]

@danilowoz No problem! I completely understand getting busy. I agree that we should probably make sure this is something that more people would want. That said, I've been giving it more thought and I'm not sure the solution in this PR is the right way to go.

It feels overly complicated.

You're also right that it would be fairly disruptive for what seems like little payoff. We should maybe consider trying to solve this a different way instead.

Maybe instead we could just:

1. Add a consoleCount variable to the static template
2. In the useSandpackConsole hook, add an effect that dispatches a register-console message on mount and an unregister-console message on unmount
3. In the static template we just listen to those events and increment/decrement the consoleCount
4. Use the consoleCount to conditionally add the console code to the head of the static template

Not sure how much better it would be, but it's probably a bit less disruptive since it would use new message types and wouldn't touch the listen method at all.

Either way, I think the code in this PR probably shouldn't be merged in its current state.

[SSHari]

@danilowoz I made a separate PR #943 with the changes I mentioned in the last comment. I still think waiting to make sure people want this functionality makes sense like you suggested, but I thought it might be good to at least get a feel for what the alternative solution might look like.

Feel free to close both draft PRs if we decide we don't want to move forward with this functionality.

Let me know what you think 👍.

[joshwcomeau]

Hey @danilowoz and @SSHari!

Just popping in to say that I think this would be a really valuable fix! I'm running into this issue now.

For more context, my React course has a mix of "static" and "react" sandpacks. My custom UI includes a "Console" tab, but right now, the console doesn't work for any of the static templates:

My temporary fix is to hide the console tab for all "static" sandpacks, but I'm worried this will discourage students from logging stuff / could get in the way of their understanding.

Of course, I understand that there's always a million things to do and it's necessary to prioritize ruthlessly, but I thought I'd add my two cents 😄.

Thanks to you both for all the great work you've done!

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sandpack-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 8, 2023 2:44pm

[danilowoz]

Hey @SSHari, I reverted a few things just to go for the simplest way and it looks good now. Thanks once again for putting so much effort into this! Would you like to take a quick look? Planning to merge it soon.

You all can test it here: https://sandpack-docs-1jcvhdpdb-codesandbox1.vercel.app/docs/quickstart

[SSHari]

@danilowoz Just took a look at the changes. Looks good to me. Thanks for getting this one over the finish line!