Conversation
❌ Deploy Preview for solidbase failed.
|
devagrawal09
left a comment
devagrawal09
left a comment
There was a problem hiding this comment.
Nice work overall: the remark plugin is clean with good author-facing errors, and SSR/worker cleanup are handled well. I got it running locally by linking packages/solid-repl from the playground, and all four examples on /repl work.
Blocking:
solid-replisn't declared anywhere, and the rewritten package isn't published yet (npm still has the old 0.26.0), so CI can't build it.pnpm-lock.yamlisn't updated for the newdev/deps, so frozen installs fail.- The plugin is registered unconditionally (inline comment).
Also: no tests for the remark plugin or the CSS rewrite, which solidbase has for its other remark plugins.
| } | ||
|
|
||
| return [ | ||
| solidReplVitePlugin(), |
There was a problem hiding this comment.
This runs for every site using the default theme, and the plugin calls require.resolve("solid-repl/package.json") at config load, so any site without solid-repl installed fails to start. It also applies the define/dedupe settings site-wide. Could this be opt-in (config flag, or only when solid-repl resolves), with solid-repl as an optional peer dep?
| return { | ||
| // solid-repl's workers bundle Babel, which reads `process.env` at load time. | ||
| // Defining one key makes Vite create a `process` global inside workers. | ||
| define: { |
There was a problem hiding this comment.
These defines apply to the whole app, not just the workers. Worth scoping if possible, or at least gating with the opt-in above.
| // dockview theming keyed on `#app` and dark mode keyed on a `.dark` ancestor. | ||
| // Drop the reset (Repl.module.css re-applies it scoped to the REPL) and rewrite | ||
| // the selectors so the rest only affects the embedded REPL. | ||
| export function scopeSolidReplCss(code: string) { |
There was a problem hiding this comment.
Checked this against the current solid-repl bundle and it works (reset removed, #app fully rewritten, nothing global leaks). Since it pattern-matches another package's output, a unit test with a fixture would catch it silently breaking when solid-repl changes.
| const text = useThemeText(); | ||
|
|
||
| // The worker protocol is not multiplexed, so each playground gets its own workers. | ||
| const compiler = new CompilerWorker(); |
There was a problem hiding this comment.
Each REPL starts 4 workers on load (incl. TS); a page with 4 REPLs started 16, with four copies of the ~9.6 MB linter. Starting them when the REPL scrolls into view or gets focus would help a lot.
devagrawal09
left a comment
devagrawal09
left a comment
There was a problem hiding this comment.
A few more from a second pass, all reproduced locally. One correction to my earlier scopeSolidReplCss comment: no global selectors (html/:root/*) with real styles leak, but Panda's utility classes (.d_flex etc.) stay global, which only matters for host sites that also use Panda.
|
|
||
| function getTabName(code: Code, index: number, file?: VFile) { | ||
| const title = new MetaOptions(code.meta ?? "").getString("title"); | ||
| if (title) return title; |
There was a problem hiding this comment.
If the first block has a title (e.g. title="App.tsx"), no main.tsx is produced, and the preview only updates when ./main exists (solid-repl/src/components/preview.tsx:278), so it stays blank with no error. Either always name the first block main.tsx or fail when no tab is main.tsx.
| import { usePreferredLanguage } from "../client/preferred-language.js"; | ||
| import CopyPageLink from "../default-theme/components/CopyPageLink.jsx"; | ||
| import { Preview, PreviewPanel, PreviewStage } from "./components/Preview.jsx"; | ||
| import { Repl } from "./components/Repl.jsx"; |
There was a problem hiding this comment.
Because Repl is always registered, every site build emits the worker bundles even with no REPL on any page: removing repl.mdx from dev/ still produced ~19.5 MB of workers (linter 9.2 MB, tsWorker 7.0 MB, compiler 2.7 MB, formatter 0.6 MB). Same fix as the opt-in comment on index.ts.
| (typeof props.devtools === "string" && props.devtools !== "false"); | ||
|
|
||
| return ( | ||
| <ErrorBoundary |
There was a problem hiding this comment.
The workers are constructed in the component body above, so a constructor failure (or the failed dynamic import in Repl.tsx) isn't caught by this boundary and the replError fallback never shows. Wrapping <ReplClient> in the boundary from Repl.tsx would cover both.
| // solid-repl's compiled output imports panda's generated runtime, | ||
| // which the package ships in dist/styled-system. | ||
| alias: { | ||
| "styled-system": join(solidReplRoot, "dist/styled-system"), |
There was a problem hiding this comment.
This alias is global, so a host site that uses Panda's own styled-system would get the REPL's copy instead. Only matters for Panda-based sites, but worth scoping to imports from solid-repl.
2 participants
No description provided.