Skip to content

migrate from quill to quill-next#27308

Open
brrichards wants to merge 5 commits into
microsoft:mainfrom
brrichards:migrate-quill
Open

migrate from quill to quill-next#27308
brrichards wants to merge 5 commits into
microsoft:mainfrom
brrichards:migrate-quill

Conversation

@brrichards
Copy link
Copy Markdown
Contributor

@brrichards brrichards commented May 13, 2026

Summary

Swaps the quill dependency for quill-next, an API-compatible fork, in both @fluidframework/quill-react and the text-editor example. No code changes beyond import paths.

The motivating difference: quill-next does not access document at module-import time. This lets us delete the workarounds that existed to satisfy that constraint:

  • packages/framework/quill-react/src/test/mochaHooks.ts (deleted) — used to install globalJsdom() at module load and tear it down in a beforeAll root hook so per-test hooks could install a clean DOM. With no import-time DOM requirement, the file (and its RootHookObject export) has nothing to do.
  • The matching config.require = [...] line in packages/framework/quill-react/.mocharc.cjs.
  • The module-load globalJsdom() call in examples/data-objects/text-editor/src/test/mochaHooks.ts — that file now only registers the CSS loader.
  • The "Known Issues and Limitations" section in quill-react's README that warned consumers about the import-time DOM requirement.

Per-test JSDOM setup in cleanup.test.tsx, textEditor.test.tsx, and app.test.tsx is unchanged in behavior — those hooks now own DOM lifecycle end-to-end.

All previous tests pass after the package change.

Copilot AI review requested due to automatic review settings May 13, 2026 20:14
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 13, 2026

Hi! Thank you for opening this PR. Want me to review it?

Based on the diff (290 lines, 15 files), I've queued these reviewers:

  • Correctness — logic errors, race conditions, lifecycle issues
  • Security — vulnerabilities, secret exposure, injection
  • API Compatibility — breaking changes, release tags, type design
  • Performance — algorithmic regressions, memory leaks
  • Testing — coverage gaps, hollow tests

How this works

  • Adjust the reviewer set by ticking/unticking boxes above. Reviewer toggles alone don't trigger anything.

  • Tick Start review below to dispatch the review fleet.

  • After review finishes, tick Start review again to request another run — it auto-resets after each dispatch.

  • This comment updates as new commits land; your reviewer selections are preserved.

  • Start review

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 13, 2026

🔭 PR Review Fleet Report

Note

This report is generated by an experimental AI review fleet and is provided as a beta feature. Findings are a starting point for discussion, not a gate. Use your own judgement.

Verdict: ⚠️ Approve with Suggestions

0 Alert, 0 Stop, 1 Caution

Findings

Sev # Area File What Fix
🚧 Caution M1 Correctness packages/framework/quill-react/src/formatted/quillFormattedView.tsx:22 const Delta = DeltaPackage as unknown as typeof DeltaPackage.default silently assumes that the default import of @quill-next/delta-es IS the Delta constructor at runtime (i.e., a true ESM default export), not an interop wrapper with a nested .default property. Under Node16 module resolution (used throughout this repo), if @quill-next/delta-es ships a CommonJS build whose interop layer places the class under .default, then DeltaPackage would be the module-exports object rather than the constructor. Every call to new Delta() — in clipboardFormatMatcher (line ~164), in the handleTextChange op-processing loop, and in the corresponding test file — would throw TypeError: DeltaPackage is not a constructor, silently breaking all paste operations with formatting and all text-change processing. The cast bypasses all compile-time protection (skipLibCheck: true further prevents the type mismatch from surfacing), so a future @quill-next/delta-es patch release that changes export format would not produce any TS error. Add a runtime guard: after the import, assert typeof DeltaPackage === 'function' (or typeof Delta === 'function') and throw a descriptive error if the assumption is violated. Alternatively, adopt a defensive form that works regardless of whether the default export is nested: const Delta = (DeltaPackage as { default?: unknown }).default ?? DeltaPackage cast to the constructor type, which is the same pattern used by the old quill-delta import (DeltaPackage.default). The same fix applies to packages/framework/quill-react/src/test/textEditor.test.tsx:34.

View workflow run

@brrichards brrichards requested a review from a team May 13, 2026 20:18
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates the Quill-based React integration and text-editor example from quill to the API-compatible quill-next fork, removing test workarounds that were only needed because quill accessed the DOM at import time.

Changes:

  • Replaces quill dependencies/imports/CSS paths with quill-next.
  • Removes import-time JSDOM setup hooks from quill-react and simplifies text-editor test setup.
  • Updates README wording to describe the quill-next-based integration.

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pnpm-lock.yaml Updates lockfile entries from quill to quill-next and its transitive delta package.
packages/framework/quill-react/package.json Swaps package dependency from quill to quill-next.
packages/framework/quill-react/README.md Updates package description and removes obsolete DOM import-time limitation.
packages/framework/quill-react/.mocharc.cjs Removes explicit test hook require for import-time JSDOM setup.
packages/framework/quill-react/src/plain/quillView.tsx Updates Quill import to quill-next.
packages/framework/quill-react/src/formatted/quillFormattedView.tsx Updates Quill import to quill-next while keeping standalone quill-delta.
packages/framework/quill-react/src/test/cleanup.test.tsx Updates test import to quill-next.
packages/framework/quill-react/src/test/textEditor.test.tsx Removes obsolete comment about import-time JSDOM setup.
packages/framework/quill-react/src/test/mochaHooks.ts Deletes no-longer-needed root hook plugin.
packages/framework/quill-react/src/test/tsconfig.json Removes deleted mocha hook from test include list.
examples/data-objects/text-editor/package.json Swaps example dependency from quill to quill-next.
examples/data-objects/text-editor/src/app.tsx Updates stylesheet import path to quill-next.
examples/data-objects/text-editor/.mocharc.cjs Updates test setup comment to reflect CSS-loader-only hook.
examples/data-objects/text-editor/src/test/mochaHooks.ts Removes import-time JSDOM setup and keeps CSS loader registration.
examples/data-objects/text-editor/src/test/app.test.tsx Adds suite-scoped JSDOM setup for rendering tests.
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

Comment thread packages/framework/quill-react/package.json Outdated
brrichards and others added 4 commits May 13, 2026 13:27
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  288641 links
    1922 destination URLs
    2172 URLs ignored
       0 warnings
       0 errors


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.

2 participants