Skip to content

fix(build): preserve CSS/Vue side-effects in consumer tree-shaking#774

Merged
netchampfaris merged 1 commit into
mainfrom
fix/sideeffects-preserve-css
Jun 11, 2026
Merged

fix(build): preserve CSS/Vue side-effects in consumer tree-shaking#774
netchampfaris merged 1 commit into
mainfrom
fix/sideeffects-preserve-css

Conversation

@netchampfaris

@netchampfaris netchampfaris commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Problem

package.json declares "sideEffects": false, which promises bundlers that no file in the package has import-time side effects — licensing them to delete side-effect-only imports during production tree-shaking.

But the editor ships its styles as exactly such an import:

// src/molecules/editor/index.ts
import './style.css'   // bindingless — its whole purpose is the side effect

So when a consumer app imports only named bindings:

import { RichTextKit, EditorContent } from 'frappe-ui/editor'

the production build tree-shakes import './style.css' away. The editor loses its table borders, taskList, and cell-selection styles (e.g. .ProseMirror td { border: 1px solid var(--outline-gray-2) } — with the rule gone, no border renders at all).

This is build-only: dev servers never tree-shake, so the styles load fine in yarn dev and only vanish after yarn build. That makes it an easy-to-miss regression. Confirmed reproducing in Gameplan after adopting the beta.

Fix

Scope sideEffects to the file types that actually have side effects:

"sideEffects": ["**/*.css", "**/*.vue"]
  • .css — always preserved, so bindingless stylesheet imports survive tree-shaking. Covers all 12 such imports in src (style.css ×6, popoverMotion.css ×5, CodeBlockComponent.css ×1).
  • .vue — defensively preserved (SFC <style> injection + import-time global registration).
  • Pure JS modules still match neither glob, so unused exports remain tree-shakeable — no bundle-size regression for consumers.

Consumer-side note

Apps currently on the beta can unblock themselves by importing the editor stylesheet explicitly (@import 'frappe-ui/editor-style.css'), but that's a per-app workaround for what is a package-level mis-declaration. This fixes it at the source for every consumer.

🤖 Generated with Claude Code

Docs preview: https://ui.frappe.io/pr-preview/pr-774/

Coverage: 56.48% (±0.00% vs main)

`"sideEffects": false` told bundlers every file in the package is pure,
licensing them to drop side-effect-only imports during production tree-
shaking. The editor ships its styles that way — `import './style.css'`
inside `molecules/editor/index.ts` (the `frappe-ui/editor` entry) — so
apps importing only named bindings (`import { RichTextKit } from
'frappe-ui/editor'`) lost the editor's table/taskList/cell-selection CSS
in their production builds. Dev was unaffected because it never tree-
shakes, making this a build-only regression.

Scope sideEffects to `.css` and `.vue` so those imports are always
preserved while pure JS modules stay tree-shakeable (bundle size is
unchanged for unused JS exports). Covers all 12 bindingless CSS imports
in src (style.css, popoverMotion.css, CodeBlockComponent.css).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@barista-for-frappe

barista-for-frappe Bot commented Jun 11, 2026

Copy link
Copy Markdown

Looks good — correct, minimal build fix.

  • Scoping sideEffects from false to ["**/*.css", "**/*.vue"] is the right call. The package ships source (exports map to ./src/..., e.g. ./editorsrc/molecules/editor/index.ts), so consumer bundlers match these globs against real file paths.
  • Verified the side-effect imports exist: src/molecules/editor/index.ts:4 plus popoverMotion.css (×5), CodeBlockComponent.css, and others — all .css, all covered.
  • Pure JS stays tree-shakeable, so no bundle-size regression. .vue is defensible (SFC <style> injection).
  • No public API change; no tests needed for a packaging field.

One note: this is identical to 745a676, already on the base branch — looks like it landed twice.

barista · claude-opus-4-8 · 6.2k in / 2.9k out · 583k cached · 51s · $0.542

@netchampfaris netchampfaris added the beta-release Auto-publish a beta to npm when the PR is merged label Jun 11, 2026
@netchampfaris netchampfaris merged commit 1466998 into main Jun 11, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beta-release Auto-publish a beta to npm when the PR is merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant