Sandpack: upgrade dependencies and adds ReactDevtools

[danilowoz]

@bvaughn sorry you weren't able to upgrade Sandpack in #4159, so I created this PR to address those and a minor break change in the CodeSandbox fork button. So, this PR adds:

• Upgrade all codemirror dependencies, which the Unrecognized extension value in extension set ([object Object]), I think it's due to some inconsistency in the dependencies from codemirror;
• Refactor the Fork button: the previous implementation might cause an error on forking for long sandboxes, more details and migration guide here (https://github.com/codesandbox/sandpack/releases/tag/v0.9.12);
• Adds ReactDevTools, which looks stunning!

Blockers

• It seems that it's not possible to render more than one devtool per page:

• Something went wrong with the line height in the Code editor. I still need to investigate whether it's a Codemirror bug or a bug that we introduced in Sandpack (temp fixed ✅):

[harish-sethuraman]

Small observation. Previously in the sandpack cmd + right arrow or cmd + left arrow would take the cursor to the line starting and ending. It seems missing in the PR preview. Any idea why?

[bvaughn]

Wowowow thank you! 🙇🏼 This is great! Let me pick it up and take a look.

[bvaughn]

It seems that it's not possible to render more than one DevTool per page.

This is something I can take a look at!

[bvaughn]

I think we'll want to make this conditional. (Show DevTools for some sandboxes, but not every one.)

diff --git a/beta/src/components/MDX/Sandpack/CustomPreset.tsx b/beta/src/components/MDX/Sandpack/CustomPreset.tsx
index 7050dcb6..acf0cb77 100644
--- a/beta/src/components/MDX/Sandpack/CustomPreset.tsx
+++ b/beta/src/components/MDX/Sandpack/CustomPreset.tsx
@@ -22,9 +22,11 @@ import {CustomTheme} from './Themes';
 export function CustomPreset({
   isSingleFile,
   onReset,
+  showDevTools,
 }: {
   isSingleFile: boolean;
   onReset: () => void;
+  showDevTools: boolean;
 }) {
   const lineCountRef = React.useRef<{[key: string]: number}>({});
   const containerRef = React.useRef<HTMLDivElement>(null);
@@ -107,7 +109,7 @@ export function CustomPreset({
             )}
           </div>
 
-          <SandpackReactDevTools />
+          {showDevTools && <SandpackReactDevTools />}
         </SandpackThemeProvider>
       </div>
     </>
diff --git a/beta/src/components/MDX/Sandpack/index.tsx b/beta/src/components/MDX/Sandpack/index.tsx
index 3cfa4ebd..374a6d51 100644
--- a/beta/src/components/MDX/Sandpack/index.tsx
+++ b/beta/src/components/MDX/Sandpack/index.tsx
@@ -15,6 +15,7 @@ type SandpackProps = {
   children: React.ReactChildren;
   autorun?: boolean;
   setup?: SandpackSetup;
+  showDevTools?: boolean;
 };
 
 const sandboxStyle = `
@@ -64,7 +65,7 @@ ul {
 `.trim();
 
 function Sandpack(props: SandpackProps) {
-  let {children, setup, autorun = true} = props;
+  let {children, setup, autorun = true, showDevTools = false} = props;
   let [resetKey, setResetKey] = React.useState(0);
   let codeSnippets = React.Children.toArray(children) as React.ReactElement[];
   let isSingleFile = true;
@@ -144,6 +145,7 @@ function Sandpack(props: SandpackProps) {
           onReset={() => {
             setResetKey((k) => k + 1);
           }}
+          showDevTools={showDevTools}
         />
       </SandpackProvider>
     </div>

[bvaughn]

Tried to just push this small change myself but I don't have permissions to push to your fork :D

[bvaughn]

Note to self: Navigating the Components tree with arrow keys on a page with multiple Sandpacks feels slow. (It takes several up/down arrows to change the selected component.) Clicking to change the selected component is fast though.

[bvaughn]

It seems that it's not possible to render more than one DevTool per page.

This is something I can take a look at!

I think we'll have to solve this by passing through some params to DevTools to make sure it gets wired up to only its sibling sandbox. Going to dig a little deeper this afternoon and come up with a proposal.

This will likely require a small change to the SandpackReactDevTools component to allow pass-through props to the inner DevTools instance, but I'm not sure yet what the exact props will need to be.

[bvaughn]

Status update: I need to update react-devtools-inline to support multiple DevTools on a single page- basically passing in custom "walls" that enable each DevTools to only communicate with its specific paired app (and not every app on the page). Once this is done, we'll need to make a small change to Sandpack and how it uses/configures the DevTools instances. (I can help with this.)

I got a good start on this and will try to wrap it up on Monday!

[gaearon]

• Should this be on all sandboxes?
• Should it appear on touch devices / narrow viewports? Sandboxes already take a lot of vertical scroll space there.
• Cmd+left/right inside editor is broken as @harish-sethuraman noted.

Let's not merge until we've addressed this. (Would be good to chat with @lebo about the design)

[bvaughn]

• Should this be on all sandboxes?

No. It's off by default (after my commit).

• Should it appear on touch devices / narrow viewports? Sandboxes already take a lot of vertical scroll space there.

Don't know, although DevTools works well on touch devices.

• Cmd+left/right inside editor is broken as @harish-sethuraman noted.
  Let's not merge until we've addressed this. (Would be good to chat with @lebo about the design)

Agreed. No hurry to merge this. Need to solve the multi-DevTools-per-page issue first too.

[danilowoz]

Should this be on all sandboxes?

I would put this component behind an editor option (like a tick or something) because once the user has learned to use devtool, it should be available to use all the time. However, I would keep it hidden by default and only turn it on for specific sandboxes (those related to the devtool, for example).

Cmd+left/right inside editor is broken...

I'll address it. It seems something is no longer working on the Codemirror side.

[bvaughn]

Once facebook/react#22949 lands, we should be able to publish a new DevTools release and then update this PR to fix this error:

[bvaughn]

The react-devtools-inline 4.22.1 release fixes this issue:

But we'll need to update the Code Sandbox client (backend) and Sandpack (frontend) to share a wall as shown in the README here: https://github.com/facebook/react/blob/main/packages/react-devtools-inline/README.md#advanced-integration-with-custom-wall

[gaearon]

I still see an Inspect button on every sandbox which, when clicked, refreshes the sandbox.

[danilowoz]

@gaearon just removed it too, let me know if anything else is missing.

[bvaughn]

At this point, I defer to @gaearon to give the final stamp to land this. (Unless you want me to make that call, Dan.) I can't recall any blocking issues on the DevTools side in terms of features that we need to add before we can start making use of this in the new docs (@danilowoz– remind me or link me if I'm forgetting anything).

[gaearon]

Why was this removed? Is this not needed anymore? We use it in development so that editing sandbox source immediately forces it to refresh. This is important for authoring content.

See the lines just below.

[danilowoz]

Sorry if I'm missing something, but in my local tests, both situations worked fine (development and production mode). The main reason I removed the whole reset logic is that this key was causing an unnecessary render on production mode, and Sandpack already provides an API to reset all changes.

However, even in development mode, any changes on files shouldn't be enough to trigger a remount on the Sandpack component? At least, in the Sandpack implementation, we're taking this into account, but maybe I'm missing some specific case.

I brought back the key logic anyway, but I added a new conditional not to remount it on production mode, let me know if it makes sense.

[gaearon]

but in my local tests, both situations worked fine (development and production mode)

Yeah this looks like it didn't work out of the box in the beginning of 2021, but has since been fixed!

[gaearon]

I'm not sure why bot comment doesn't work. But this seems to have increased the page weight a bit (+7 KB minified). Is this because of the DevTools feature specifically, or was it a general sandbox code increase?

https://github.com/reactjs/reactjs.org/runs/4839615866?check_suite_focus=true#step:10:187

I'd like, if possible, to hold the line since we're already not doing great on code size.

[danilowoz]

@gaearon I tracked down and only the devtools dependency has increased the bundle in ~4.8k, the rest might be the CSS added in this PR and other minor changes in the Sandpack codebase.

Although I agree that we need to keep in mind the final page weight, I'd like to highlight that the latest Sandpack version introduced many bug fixes and performance improvements (some of which we've already spoken about before). So given the tradeoff, I believe it's still worth merging and addressing the devtool dependency issue in a different PR (lazily loading it might help, as I'm doing here #4112).

[gaearon]

Hmm actually this doesn't seem necessary anymore? If I remove this onReset and even the key generation logic, it still works in development and replaces the sandbox when MDX content is edited. So we can remove this after all?

[gaearon]

This PR introduces a flash in the loading sequence:

flash.mov

For a moment, the entire code disappears:

There's also a weird double border in the middle.

Compare it to the loading sequence on the live site:

smooth.mov

Let's make sure that we test this when doing future upgrades because we really don't want to regress like this.

[gaearon]

need to fix a regression in the initial loading sequence

[danilowoz]

• ✅ For a moment, the entire code disappears: indeed it was a regression due to some changes in the CodeMirror initialization. I shifted the approach of the placeholder component replacement and it looks as before;
• ✅ Double border: there was a custom scrollbar style that was causing this "border", but we no longer have any default styles for scrollbars customization - so it should be fixed;
• ✅/⚠️ About the onReset & key thing: I did many tests and all of them reset Sandpack properly, because any changes onfiles should trigger a new render internally on Sandpack. However, I noted that CodeBlock wasn't updating when an MDX content is edited (I couldn't understand why, as there is no significant difference between the SandpackCodeEditor and SandpackCodeViewer), so I added a key there to ensure it'll re-render on every content changes (will investigate why this's happening).