Sandpack: upgrade dependencies and adds ReactDevtools #4161

danilowoz · 2021-12-10T10:10:59Z

@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 · 2021-12-10T12:19:30Z

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 · 2021-12-10T16:12:19Z

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

bvaughn · 2021-12-10T16:22:07Z

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 · 2021-12-10T16:27:27Z

beta/src/components/MDX/Sandpack/CustomPreset.tsx

@@ -104,6 +106,8 @@ export function CustomPreset({
              </button>
            )}
          </div>
+
+          <SandpackReactDevTools />


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>

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

bvaughn · 2021-12-10T16:31:50Z

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 · 2021-12-10T16:39:31Z

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 · 2021-12-10T21:08:09Z

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 · 2021-12-11T16:41:34Z

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 · 2021-12-12T03:20:16Z

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 · 2021-12-12T17:48:47Z

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 · 2021-12-13T15:03:08Z

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 · 2021-12-14T23:28:10Z

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 · 2022-01-14T17:35:58Z

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

danilowoz · 2022-01-17T10:51:39Z

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

bvaughn · 2022-01-18T13:51:22Z

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 · 2022-01-18T18:35:30Z

beta/src/components/MDX/Sandpack/index.tsx

@@ -121,29 +124,17 @@ function Sandpack(props: SandpackProps) {
    hidden: true,
  };

-  let key = String(resetKey);


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.

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.

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 · 2022-01-18T18:41:06Z

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 · 2022-01-19T11:38:05Z

@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).

beta/src/components/MDX/Sandpack/OpenInCodeSandboxButton.tsx

beta/src/components/Icon/IconNewPage.tsx

gaearon · 2022-01-19T16:11:22Z

beta/src/components/MDX/Sandpack/index.tsx

          onReset={() => {
-            setResetKey((k) => k + 1);
+            /**


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 · 2022-01-19T16:26:18Z

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 · 2022-01-24T11:33:40Z

✅ 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).

danilowoz added 4 commits December 10, 2021 09:31

upgrade sandpack dependencies

059d8c7

update csb button

c5ebd61

remove unnecessary ref

2d4e27b

add react dev tools

5202a8f

facebook-github-bot added the CLA Signed label Dec 10, 2021

vercel bot deployed to Preview – reactjs-org December 10, 2021 10:11 View deployment

vercel bot had a problem deploying to Preview – beta-reactjs-org December 10, 2021 10:11 Failure

github-actions bot added the beta label Dec 10, 2021

remove duplicated/unnecessary element

50498f5

vercel bot deployed to Preview – reactjs-org December 10, 2021 10:47 View deployment

vercel bot deployed to Preview – beta-reactjs-org December 10, 2021 10:47 View deployment

fix: codemirror gutter

c53ec60

vercel bot deployed to Preview – reactjs-org December 10, 2021 11:46 View deployment

vercel bot deployed to Preview – beta-reactjs-org December 10, 2021 11:46 View deployment

bvaughn self-requested a review December 10, 2021 16:11

bvaughn reviewed Dec 10, 2021

View reviewed changes

bvaughn mentioned this pull request Dec 13, 2021

DevTools: Support mulitple DevTools instances per page facebook/react#22949

Merged

fix codemirror gutter

b5adb42

vercel bot deployed to Preview – reactjs-org December 15, 2021 13:14 View deployment

vercel bot deployed to Preview – beta-reactjs-org January 17, 2022 10:48 View deployment

vercel bot deployed to Preview – reactjs-org January 17, 2022 10:48 View deployment

vercel bot deployed to Preview – beta-reactjs-org January 17, 2022 10:49 View deployment

remove inspect button

bfe9ad9

vercel bot deployed to Preview – reactjs-org January 17, 2022 10:50 View deployment

vercel bot deployed to Preview – beta-reactjs-org January 17, 2022 10:50 View deployment

gaearon reviewed Jan 18, 2022

View reviewed changes

bring back key logic

a91d10e

harish-sethuraman reviewed Jan 19, 2022

View reviewed changes

beta/src/components/MDX/Sandpack/OpenInCodeSandboxButton.tsx Outdated Show resolved Hide resolved

harish-sethuraman reviewed Jan 19, 2022

View reviewed changes

beta/src/components/MDX/Sandpack/OpenInCodeSandboxButton.tsx Outdated Show resolved Hide resolved

harish-sethuraman reviewed Jan 19, 2022

View reviewed changes

beta/src/components/Icon/IconNewPage.tsx Outdated Show resolved Hide resolved

refactor: icon align

a965508

vercel bot deployed to Preview – beta-reactjs-org January 19, 2022 15:23 View deployment

gaearon reviewed Jan 19, 2022

View reviewed changes

vercel bot deployed to Preview – reactjs-org January 19, 2022 16:20 View deployment

gaearon requested changes Jan 19, 2022

View reviewed changes

danilowoz added 3 commits January 24, 2022 09:32

conflict: merge from master

2a1cf07

update sandpack

32ba5ff

logic to keep the content synced

0686663

vercel bot deployed to Preview – reactjs-org January 24, 2022 11:33 View deployment

vercel bot deployed to Preview – beta-reactjs-org January 24, 2022 11:33 View deployment

danilowoz requested a review from gaearon January 24, 2022 11:35

gaearon merged commit d80ef56 into reactjs:main Jan 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sandpack: upgrade dependencies and adds ReactDevtools #4161

Sandpack: upgrade dependencies and adds ReactDevtools #4161

danilowoz commented Dec 10, 2021 •

edited

Loading

harish-sethuraman commented Dec 10, 2021

bvaughn commented Dec 10, 2021

bvaughn commented Dec 10, 2021

bvaughn Dec 10, 2021

bvaughn Dec 10, 2021

bvaughn commented Dec 10, 2021

bvaughn commented Dec 10, 2021 •

edited

Loading

bvaughn commented Dec 10, 2021

gaearon commented Dec 11, 2021

bvaughn commented Dec 12, 2021

danilowoz commented Dec 12, 2021 •

edited

Loading

bvaughn commented Dec 13, 2021

bvaughn commented Dec 14, 2021 •

edited

Loading

gaearon commented Jan 14, 2022

danilowoz commented Jan 17, 2022

bvaughn commented Jan 18, 2022

gaearon Jan 18, 2022 •

edited

Loading

danilowoz Jan 19, 2022

gaearon Jan 19, 2022

gaearon commented Jan 18, 2022

danilowoz commented Jan 19, 2022

gaearon Jan 19, 2022

gaearon commented Jan 19, 2022

gaearon left a comment

danilowoz commented Jan 24, 2022 •

edited

Loading

Sandpack: upgrade dependencies and adds ReactDevtools #4161

Sandpack: upgrade dependencies and adds ReactDevtools #4161

Conversation

danilowoz commented Dec 10, 2021 • edited Loading

harish-sethuraman commented Dec 10, 2021

bvaughn commented Dec 10, 2021

bvaughn commented Dec 10, 2021

bvaughn Dec 10, 2021

Choose a reason for hiding this comment

bvaughn Dec 10, 2021

Choose a reason for hiding this comment

bvaughn commented Dec 10, 2021

bvaughn commented Dec 10, 2021 • edited Loading

bvaughn commented Dec 10, 2021

gaearon commented Dec 11, 2021

bvaughn commented Dec 12, 2021

danilowoz commented Dec 12, 2021 • edited Loading

bvaughn commented Dec 13, 2021

bvaughn commented Dec 14, 2021 • edited Loading

gaearon commented Jan 14, 2022

danilowoz commented Jan 17, 2022

bvaughn commented Jan 18, 2022

gaearon Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

danilowoz Jan 19, 2022

Choose a reason for hiding this comment

gaearon Jan 19, 2022

Choose a reason for hiding this comment

gaearon commented Jan 18, 2022

danilowoz commented Jan 19, 2022

gaearon Jan 19, 2022

Choose a reason for hiding this comment

gaearon commented Jan 19, 2022

gaearon left a comment

Choose a reason for hiding this comment

danilowoz commented Jan 24, 2022 • edited Loading

danilowoz commented Dec 10, 2021 •

edited

Loading

bvaughn commented Dec 10, 2021 •

edited

Loading

danilowoz commented Dec 12, 2021 •

edited

Loading

bvaughn commented Dec 14, 2021 •

edited

Loading

gaearon Jan 18, 2022 •

edited

Loading

danilowoz commented Jan 24, 2022 •

edited

Loading