Skip to content

Migrate scrabble module to Conductor#792

Draft
Shrey5132 wants to merge 4 commits into
source-academy:conductor-migrationfrom
Shrey5132:feat/migrate-scrabble
Draft

Migrate scrabble module to Conductor#792
Shrey5132 wants to merge 4 commits into
source-academy:conductor-migrationfrom
Shrey5132:feat/migrate-scrabble

Conversation

@Shrey5132

@Shrey5132 Shrey5132 commented Jul 13, 2026

Copy link
Copy Markdown

Description

Fixes #782

Migrates the scrabble module to be Conductor-ready. scrabble has no tab/visualization component, so this is scoped entirely to src/bundles/scrabble/.

Unlike repeat/binary_tree/midi, scrabble exports four static word/letter lists, not functions - there's nothing for the usual exportedNames/@moduleMethod closure path to wrap. index.ts is now a BaseModulePlugin subclass that pushes all four exports in an initialise() override, built as real DataType.ARRAYs via array_make/array_set.

DataType.OPAQUE was the initial choice here (see review history) - IDataHandler.array_make has no bulk constructor, so populating an array is one array_set call per element, and scrabble_words alone is 172,820 entries. That looked prohibitive on paper. It wasn't measured, though, and the actual cost (via @sourceacademy/modules-testplugin's TestDataHandler) is ~246ms one-time for both scrabble_words and the nested scrabble_letters combined - negligible at module-load time, and a module's evaluator runs in the same execution context (the Runner), so there's no postMessage boundary inflating that further in production. opaque_make stays the right call for genuinely opaque payloads (e.g. binary_tree's node values, rune's whole Rune object) - the difference is scrabble's data isn't opaque at all, it's a plain word list, so wrapping it that way just cost students print/indexing/iteration for no real reason.

This also follows source-academy/py-slang#217's module-interop fixes and the resulting team decision that Python lists / JS arrays should be the only built-in data structure modules hand back, rather than reaching for opaque_make on anything compound.

functions.ts is untouched - it's pure data derivation (map/filter over the word list), nothing conductor-specific about it.

Unrelated bug fixed along the way

The existing scrabble_words matches snapshot / scrabble_letters matches snapshot tests hung vitest indefinitely - confirmed via bisection against repeat's suite (clean, 1.2s) and this module's new test in isolation via .only (clean, 2.76s). Snapshotting a 172,820-entry array (and the nested scrabble_letters) produced a 2.1M-line .snap file that vitest's serializer choked on. Dropped those two tests and deleted the stale snapshot file; the existing index spot-checks (scrabble_words[12], scrabble_letters[100000]) already cover the full arrays cheaply, and the _tiny variants (~1,728 entries) still get proper snapshots (regenerated, 2 written).

Testing

  • yarn workspace @sourceacademy/bundle-scrabble run tsc - passes
  • yarn workspace @sourceacademy/bundle-scrabble run lint - passes
  • yarn workspace @sourceacademy/bundle-scrabble run test - 5/5 passing, against @sourceacademy/modules-testplugin's TestDataHandler
  • yarn workspace @sourceacademy/bundle-scrabble run build - produces build/bundles/scrabble.js

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

scrabble exports four static word/letter lists rather than functions,
so there's nothing for the usual exportedNames/@moduleMethod closure
path to wrap. Exposes them via DataType.OPAQUE (opaque_make in an
initialise() override) instead of DataType.ARRAY, since array_make has
no bulk constructor and the full word list is 172,820 entries.

Also drops the two full-array snapshot tests (scrabble_words/
scrabble_letters) - snapshotting 172,820 entries produced a
multi-million-line .snap file that hung vitest's serializer. The
existing index spot-checks already cover the full arrays; only the
~1,728-entry _tiny variants are snapshotted now.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the scrabble bundle to export a ScrabbleModulePlugin extending BaseModulePlugin instead of directly exporting the word lists. The word lists are now exposed as opaque values asynchronously during initialization. Additionally, snapshot tests for the full-sized word lists were removed to prevent test runner hangs, and a new test was added to verify the plugin's initialization behavior. Feedback is provided to optimize the initialization performance by executing the asynchronous opaque_make calls concurrently using Promise.all instead of sequentially.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/bundles/scrabble/src/index.ts
@Shrey5132
Shrey5132 marked this pull request as draft July 13, 2026 22:46
The four exports are independent, so awaiting them one at a time added
needless latency. Addresses gemini-code-assist review comment on source-academy#792.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@Shrey5132
Shrey5132 requested a review from Akshay-2007-1 July 13, 2026 22:56
Shrey Jain and others added 2 commits July 16, 2026 16:38
Resolves conflicts in scrabble's package.json/yarn.lock (typescript
version now tracks the workspace catalog instead of a local pin,
matching upstream's cleanup). Also adds the `override` modifier to
`exportedNames`/`channelAttach` in index.ts - conductor-migration
picked up `noImplicitOverride` since this branch diverged, same fix
already applied to `repeat`'s index.ts.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
array_make's lack of a bulk constructor (one array_set call per
element) looked prohibitive for 172,820 words, so this used
opaque_make instead. That was never measured, and the actual cost is
~246ms one-time (TestDataHandler, same-thread as the evaluator - no
postMessage boundary between a module and its evaluator) for both
scrabble_words and the nested scrabble_letters. Cheap enough that
there's no reason to give up real indexing/print/iteration for it.

Follows from source-academy/py-slang#217's module-interop fixes and
the team's decision that Python lists/JS arrays should be the only
built-in data structure modules hand back - opaque_make stays reserved
for genuinely opaque payloads (e.g. binary_tree's node values), not
plain collections.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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