Skip to content

refactor(cli): pass through reactStrictMode to let studio default take effect#1147

Open
jordanl17 wants to merge 2 commits into
mainfrom
sapp-3841
Open

refactor(cli): pass through reactStrictMode to let studio default take effect#1147
jordanl17 wants to merge 2 commits into
mainfrom
sapp-3841

Conversation

@jordanl17
Copy link
Copy Markdown
Member

@jordanl17 jordanl17 commented Jun 1, 2026

Description

Defers the reactStrictMode default for sanity dev to the sanity package instead of having the CLI decide it. getDevServerConfig no longer forces a value when reactStrictMode is unset, and getEntryModule emits reactStrictMode: undefined so renderStudio's own destructuring default applies. That default is off in Studio v5 and on in Studio v6, so the on-by-default behavior now lives entirely in sanity@6 and reaches users only when they upgrade the Studio. Explicit config (reactStrictMode: false or true) and the SANITY_STUDIO_REACT_STRICT_MODE env var still take precedence. Production builds (sanity build) are unaffected. Part of SAPP-3841.

This reverses the earlier approach on this branch. Previously the CLI flipped the default itself (cliConfig?.reactStrictMode ?? true), but because sanity@5 depends on @sanity/cli via ^6.x, a minor CLI release would have leaked StrictMode-on to existing sanity@5 users running sanity dev. Making the CLI a pass-through keeps this change non-breaking for current sanity@5 users and confines the breaking default to the sanity@6 major (sanity-io/sanity#12916), which also covers the component audit.

What to review

  • getDevServerConfig.ts - drops the ?? true fallback; the resolved value is now boolean | undefined.
  • getEntryModule.ts - emits the literal undefined instead of coercing with Boolean() when reactStrictMode is unset, so the generated entry lets renderStudio's default decide.
  • DevServerOptions / RuntimeOptions - reactStrictMode widened to boolean | undefined (internal types, not part of the public export surface).
  • Confirm the opt-out paths are intact: explicit config and the env var still emit concrete booleans, and sanity build still passes a literal false.

Testing

Updated getDevServerConfig tests: the unset case now resolves to undefined (deferring to the studio), with an added explicit reactStrictMode: true config case; the env-var-true, env-var-false, and explicit-false cases are unchanged. Added a new getEntryModule test asserting the generated entry contains reactStrictMode: undefined when unset (for both the config and no-config templates), concrete true/false when set, and never references reactStrictMode for the app entry. All pass; types and format clean.


Note

Medium Risk
Changes default dev behavior when neither config nor env is set (now follows Studio major version), though explicit overrides remain; avoids turning StrictMode on for existing sanity@5 projects via a CLI-only default.

Overview
For sanity dev, the CLI stops choosing a default for reactStrictMode when it is omitted from sanity.cli.ts and SANITY_STUDIO_REACT_STRICT_MODE is not set. getDevServerConfig now passes through cliConfig?.reactStrictMode (including undefined) instead of coercing with Boolean(), and getEntryModule writes the literal reactStrictMode: undefined in the generated runtime entry instead of false, so renderStudio in the installed sanity package applies its own default (off on v5, on on v6).

Explicit reactStrictMode: true|false in CLI config and the env var still resolve to concrete booleans. Internal types (DevServerOptions, RuntimeOptions, getEntryModule) accept boolean | undefined. CliConfig JSDoc now documents default true for the option when documented at the config layer. New tests cover dev resolution and generated entry output; a changeset records the @sanity/cli minor behavior note.

Reviewed by Cursor Bugbot for commit 6bb3bed. Bugbot is set up for automated code reviews on this repo. Configure here.

@claude
Copy link
Copy Markdown

claude Bot commented Jun 1, 2026

Claude finished @jordanl17's task in 3m 12s —— View job


Review

  • Read changed files via git diff origin/main...HEAD
  • Review getDevServerConfig.ts default flip and tests
  • Review cliConfig.ts JSDoc update
  • Review getEntryModule.ts / writeSanityRuntime.ts / devServer.ts changes
  • Review new getEntryModule.test.ts
  • Verify changeset is correct and follows product-copy conventions
  • Post review feedback

Issues

1. JSDoc claim doesn't match runtime behaviorpackages/@sanity/cli-core/src/config/cli/types/cliConfig.ts:88

The comment says Defaults to true. but, after commit 6bb3bed, the CLI no longer forces a value — it passes undefined through to renderStudio, and the changeset itself notes the resolved default is off in Studio v5, on in Studio v6. The JSDoc is therefore inaccurate for v5 consumers (where the field is still effectively a boolean | undefined that defaults to false). Either reword to match the changeset, e.g. "Defaults to the value chosen by renderStudio — off in Studio v5, on in Studio v6," or drop the wording about a fixed default entirely. Fix this →

2. PR description is stale relative to the second commit

The PR body still says getDevServerConfig resolves reactStrictMode to true when unset. That was the first commit; commit 6bb3bed changed it to pass undefined so renderStudio decides. The changeset is correct; the PR description should be updated so reviewers/release notes aren't misled.

3. Changeset summary is too long for repo convention.changeset/strict-mode-default-dev.md

Recent changesets in this repo are one short sentence (claude-code-mcp-oauth-only.md, sour-zebras-share.md, pr-1126.md). This one is five sentences and reads more like release notes than a changeset entry. Suggested tightening:

---
'@sanity/cli': minor
---

`sanity dev` no longer forces `reactStrictMode: false` when unset; `renderStudio`'s default applies (off in Studio v5, on in Studio v6). Set `reactStrictMode` in `sanity.cli.ts` or `SANITY_STUDIO_REACT_STRICT_MODE` to override.

4. @sanity/cli-build behavior changed but isn't in the changeset.changeset/strict-mode-default-dev.md

The changeset only lists @sanity/cli, but the publishable @sanity/cli-build package also changed: writeSanityRuntime's exported signature widened (reactStrictMode: boolean | undefined), and getEntryModule now emits reactStrictMode: undefined instead of reactStrictMode: false for the same unset input. Consistent with how fifty-sheep-strive.md handled a cli-build change, this PR probably warrants a @sanity/cli-build entry (likely minor, since the signature widened) so the next publish carries the runtime change. Fix this →

5. Generated module emits literal undefined, relying on renderStudio's destructuring shapepackages/@sanity/cli-build/src/actions/build/getEntryModule.ts:53-59

The output now contains:

renderStudio(document.getElementById("sanity"), studioConfig, {reactStrictMode: undefined, basePath: "/"})

This works only as long as renderStudio applies its default via destructuring ({reactStrictMode = …}), since explicit undefined triggers the default. If anyone over in sanity ever checks 'reactStrictMode' in options or options.reactStrictMode !== undefined, the v6 default would not apply for unconfigured users. Two safer alternatives:

  • Omit the property entirely from the emitted template when unset (slightly more template work, but expresses intent directly).
  • Make the default decision in renderStudio independent of property presence (i.e. keep {reactStrictMode = isV6} = options) — the choice is in the companion feat(core): enable react strict mode by default in studio sanity#12916 PR's hands.

Worth at least a comment in getEntryModule.ts documenting the assumption, since the meaning of undefined here is now load-bearing.

6. No e2e/integration check that renderStudio actually receives undefined from a real sanity dev

The unit tests verify getEntryModule emits the literal string reactStrictMode: undefined and getDevServerConfig returns undefined, but no test exercises the full path through writeSanityRuntime → generated file → studio. Given the rollout coordination concern (this CLI ships expecting renderStudio in v6 to default on), a smoke test in packages/@sanity/cli-e2e/ against a v6 fixture would catch the case where the wiring silently drops the flag. Not a blocker if the companion repo's tests cover it.

Nits

  • The new tests in getDevServerConfig.test.ts use vi.stubEnv but I don't see a corresponding vi.unstubAllEnvs() in afterEach in this file — check that the surrounding test scaffold restores env between tests, otherwise the env var can leak into the "cliConfig unset → undefined" case and make it pass for the wrong reason.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

📦 Bundle Stats — @sanity/cli

Compared against main (7d2118d4)

@sanity/cli

Metric Value vs main (7d2118d)
Internal (raw) 2.1 KB -
Internal (gzip) 799 B -
Bundled (raw) 10.97 MB -
Bundled (gzip) 2.06 MB -
Import time 837ms +6ms, +0.7%

bin:sanity

Metric Value vs main (7d2118d)
Internal (raw) 1023 B -
Internal (gzip) 486 B -
Bundled (raw) 9.87 MB -
Bundled (gzip) 1.77 MB -
Import time 1.94s +3ms, +0.2%

🗺️ View treemap · Artifacts

Details
  • Import time regressions over 10% are flagged with ⚠️
  • Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.

📦 Bundle Stats — @sanity/cli-core

Compared against main (7d2118d4)

Metric Value vs main (7d2118d)
Internal (raw) 96.3 KB -
Internal (gzip) 22.7 KB -
Bundled (raw) 21.64 MB -
Bundled (gzip) 3.43 MB -
Import time 786ms -9ms, -1.1%

🗺️ View treemap · Artifacts

Details
  • Import time regressions over 10% are flagged with ⚠️
  • Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.

📦 Bundle Stats — create-sanity

Compared against main (7d2118d4)

Metric Value vs main (7d2118d)
Internal (raw) 908 B -
Internal (gzip) 483 B -
Bundled (raw) 931 B -
Bundled (gzip) 491 B -
Import time ❌ ChildProcess denied: node -
Details
  • Import time regressions over 10% are flagged with ⚠️
  • Sizes shown as raw / gzip 🗜️. Internal bytes = own code only. Total bytes = with all dependencies. Import time = Node.js cold-start median.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

Coverage Delta

File Statements
packages/@sanity/cli-build/src/actions/build/getEntryModule.ts 100.0% (±0%)
packages/@sanity/cli-build/src/actions/build/writeSanityRuntime.ts 95.5% (±0%)
packages/@sanity/cli/src/actions/dev/getDevServerConfig.ts 100.0% (±0%)
packages/@sanity/cli/src/server/devServer.ts 94.4% (±0%)

Comparing 4 changed files against main @ 87cd8f26282b6a57501ef78de37b66a2ebf1c26e

Overall Coverage

Metric Coverage
Statements 84.3% (±0%)
Branches 74.3% (+ 0.0%)
Functions 84.2% (±0%)
Lines 84.8% (±0%)

@jordanl17
Copy link
Copy Markdown
Member Author

Re: rollout coordination - this CLI default flip is intended to land together with the companion Studio V6 work in sanity-io/sanity#12916, which contains the component StrictMode-safety fixes (render-purity, telemetry one-shot, manifest upload cancellation, a subscription-leak fix). Both ship as part of Studio V6.

If this CLI change were to ship before those fixes are in a published sanity, the worst case is transient extra dev-only double-invocation (StrictMode is a no-op in production builds), not a runtime regression - the audit found no crash or warning-level incompatibilities, and the dev studios have run with reactStrictMode: true for some time. Still, the two are meant to release together.

@jordanl17 jordanl17 changed the title feat(cli): enable react strict mode by default in dev refactor(cli): pass through reactStrictMode to let studio default take effect Jun 1, 2026
@jordanl17 jordanl17 marked this pull request as ready for review June 2, 2026 11:37
@jordanl17 jordanl17 requested a review from a team as a code owner June 2, 2026 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant