Migrate midi module to Conductor#791
Conversation
Splits midi into a pure, evaluator-free functions.ts (unchanged signatures) and a Conductor-facing index.ts plugin, since sound and stereo_sound import midi_note_to_frequency and friends directly as plain TypeScript and sound/stereo_sound's own Source-facing APIs re-export several of these functions. Migrating index.ts's exports to require an IDataHandler would have broken both call sites immediately. - functions.ts / scales.ts / utils.ts / types.ts: untouched pure logic - conductorAdapters.ts: undecorated helpers (scale-list <-> Conductor list conversion, accidental validation) used by the plugin; kept separate from index.ts so they stay importable from vitest, which hits a decorator syntax error importing index.ts directly - index.ts: BaseModulePlugin subclass wrapping the pure functions; SHARP/FLAT/NATURAL are pushed onto `exports` directly in the constructor since BaseModulePlugin.initialise() only registers exportedNames that are functions - Includes the same __bindExportedMethods() workaround as repeat/rune/binary_tree for the unbound-method issue in BaseModulePlugin.initialise() (source-academy/conductor#41) - sound/stereo_sound's functions.ts and index.ts updated to import midi's pure functions from the new `/functions` subpath instead of the bundle root, which now exports the Conductor plugin
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the midi bundle to support the Conductor plugin architecture, separating pure MIDI functions into functions.ts and implementing the Conductor-facing plugin in index.ts. It also updates the sound and stereo_sound bundles to import directly from the new functions entry point. The review feedback highlights a potential issue where accessing the .name property of functions (such as letter_name_to_midi_note and midi_note_to_letter_name) for error reporting could fail or produce cryptic messages in minified production environments due to name mangling. It is recommended to replace these with hardcoded string literals.
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.
Function.prototype.name gets mangled under minification, which would turn these error messages into cryptic garbage like "t expects...". Two of these were carried over unchanged from the original module; the third is in the new Conductor-facing index.ts.
conductor-migration's midi had drifted from master: 6 functions
(is_note_with_octave, add_octave_to_note, get_octave, get_note_name,
get_accidental, key_signature_to_key) and input validation on the
existing ones (midi_note_to_frequency now range-checks its input) only
existed on master, added there while this migration was in flight.
Confirmed against the published docs page
(source-academy.github.io/modules/documentation/modules/midi.html)
that this is now the complete function/constant list, no more, no
less - verified end-to-end through the actual compiled bundle, not
just unit tests, driving every export exactly the way a real evaluator
calls a closure (detached, not bound to any instance).
Also fixes midi_note_to_letter_name/key_signature_to_key's accidental
parameter to match master: it's the Accidental enum value ('#'/'b'),
not the word 'flat'/'sharp' my first pass used before I'd found the
drift. midi_note_to_letter_name silently treats anything other than
exactly SHARP as flat (matching master's actual, slightly loose
behavior) rather than validating it - key_signature_to_key is the one
that validates, since its own switch has an explicit default case for
that.
Validation now uses conductor's new EvaluatorParameterTypeError /
assertNumberWithinRange (source-academy/conductor#42) in place of
modules-lib's InvalidParameterTypeError / assertNumberWithinRange,
which functions.ts can't depend on without pulling js-slang's
modules-lib re-exports (and everything under it) into sound/
stereo_sound's dependency graph transitively. Message format is
unchanged - verified identical to master's existing test expectations.
|
Is there a way to get Vitest to work with decorators? They're functionalities need to be tested anyway |
Dug into this, afaik, I dont think its possible, not with our current Vite/Vitest setup, and it's not a config issue we can fix from our side. Root cause:Vitest runs test files through Vite's SSR module pipeline, which calls ssrTransformScript → parseAstAsync (from rolldown) unconditionally, regardless of whether oxc or esbuild is set as the TS transformer. That parser doesn't support native TC39 decorator syntax, so any test file importing a decorated class (like our index.ts plugins) throws a SyntaxError before the test even runs. I confirmed esbuild alone can downlevel decorators for non-esnext targets, and traced that Vitest defaults to oxc (which doesn't downlevel decorators for any target). I patched buildtools's runVitest to actually pass through Vite config overrides and forced esbuild/oxc:false, the config verifiably took effect, but the crash persisted, tracing straight to that hard-coded parseAstAsync call in Vite's SSR internals, below any of our config knobs. I reverted that experiment. For now, decorated Conductor-facing classes (index.ts) stay thin/untested-directly, and the actual logic under test lives in undecorated files (functions.ts, conductorAdapters.ts, utils.ts). Do you have any suggestions @leeyi45? |
AaravMalani
left a comment
AaravMalani
left a comment
There was a problem hiding this comment.
LGTM except documentation and the __bindExportedMethods function! I've made a couple non-blocking comments, though
| * key_signature_to_key(FLAT, 3); // Returns "Eb", since the key of Eb has 3 flats | ||
| * ``` | ||
| */ | ||
| export function key_signature_to_key(accidental: Accidental.FLAT | Accidental.SHARP, numAccidentals: number): Note { |
There was a problem hiding this comment.
This is unrelated to the migration and my circle of fifths is rusty, but doesn't every major key have a relative minor (D <-> Bm)? Shouldn't it be made clear in the documentation that it returns the major key / have a way to return it in a scale of the user's choosing?
There was a problem hiding this comment.
Actually, all the scales are modes of the major scale, so technically we should be able to support it returning the key in any of the scales (I'm surprised natural and harmonic minor are missing from the scales)
There was a problem hiding this comment.
The current implementation of key_signature_to_key is designed to return the major key associated with a given number of accidentals, which is standard for this type of music theory utility. While it is true that every major key has a relative minor, this function is specifically intended to map key signatures to their corresponding major keys (e.g., 2 sharps -> D major). Adding support for minor keys or other modes would require an API change to specify the scale type, which is outside the scope of this migration. I have verified that the current implementation correctly maps the key signatures as expected.
There was a problem hiding this comment.
@AaravMalani you're right that all these scales are modes of the same major scale under the hood. But, this behavior's carried over unchanged from the original js-slang module though. @martin-henz, any thoughts? Should we leave it be?
There was a problem hiding this comment.
Yes every major key has a relative minor.
Natural minor is included, I presume @AaravMalani you're referring to the harmonic and melodic minor scales.
The thing is that there's a lot of scales out there (like major and minor pentatonic), we have to draw the line somewhere.
For key_signature_to_key the assumption is that it returns the major scale with that key signature. I guess we should update the documentation to reflect that.
There was a problem hiding this comment.
oh, let's not worry about consistency with musical theory at this point. Let's aim for feature parity. If there is time, we can do improvements once everything works, or do an overhaul as part of CP3108.
…back scales.ts called pair() from js-slang/dist/stdlib/list at runtime, which is unavailable under Conductor's module loader (the require() shim it's given is a no-op), throwing "Cannot read properties of undefined (reading 'pair')" for any scale function. Build the intermediate list with a plain array tuple instead, since Pair<H, T> is just [H, T] structurally. Also addresses Aarav's review comments on #791: - Removes __bindExportedMethods now that the upstream binding fix (source-academy/conductor#41) is merged and published. - Widens letter_name_to_midi_note/midi_note_to_letter_name/ letter_name_to_frequency/add_octave_to_note/get_octave/get_note_name/ get_accidental/key_signature_to_key's note/accidental parameters to plain string, removing the unsafe `as NoteWithOctave`/`as Accidental...` casts in index.ts. Doing this exposed two real validation gaps that the casts had been silently papering over: add_octave_to_note never validated `note` at all (just interpolated it into the result string), and midi_note_to_letter_name/midiNoteToNoteName treated any non-SHARP accidental as FLAT instead of rejecting it. Both now throw EvaluatorParameterTypeError for invalid input, with regression tests added.
…cy resolution scales.ts and conductorAdapters.ts only ever needed js-slang's List/Pair type shape, not js-slang itself. Replaced with a local Scale type; js-slang moves to devDependencies since only the test suite still uses it. Also: midi depended on @sourceacademy/conductor via a bare, unpinned GitHub URL, which yarn.lock had resolved and locked to a commit from before even the BaseModulePlugin binding fix (conductor#41). A real `yarn install` (not using a local portal: link for testing) was building against that stale, broken conductor entirely silently. Switched to the same versioned npm range other bundles already use (^0.7.0), and reverted the two calls to assertNumberWithinRange that had been written against conductor PR #43's still-unpublished options-object signature back to the options actually published in 0.7.0.
Yeah, dug into this a bit more. Checked the generated docs json for this bundle and every function comes back as "No description available", even though the actual JSDoc is all there in functions.ts. The doc generator reads off the index.ts wrapper methods, and those are just thin passthroughs with no JSDoc of their own, so it never sees the real docs on the underlying functions. That lines up exactly with what #765 is fixing (buildtools doc engine needs to understand the functionDeclaration/variableDeclaration decorators to trace through to the actual implementation). So this isn't something we can just patch locally in this PR, it's blocked on that landing first. Same story for binary_tree, and honestly any other module going through this migration pattern, since they all hit the same wrapper-class setup. Want to hold off merging any of these until #765 is in and we've actually confirmed docs regenerate properly for at least one of them, rather than assuming it'll just work itself out after. |
…igrate-midi Resolved conflicts by keeping midi's already-Conductor-migrated code (class-based MidiModulePlugin, conductor/common errors, js-slang-free Scale type) over conductor-migration's stale pre-migration content, same pattern as feat/migrate-binary-tree. Also fixed an unrelated import-path conflict in sound/stereo_sound's functions.ts (combining both sides' imports) and a pre-existing type error in midi's test that surfaced once real type-checking ran again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Picked up as part of merging conductor-migration in - noImplicitOverride is now enabled repo-wide, and exportedNames/channelAttach shadow members declared on BaseModulePlugin. Also fixed a test that built its input scale via js-slang's untyped list() instead of midi's own js-slang-free Scale shape, which only surfaced as a real tsc error once the merge brought stricter checking back online. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
I'm not dead set on Typedoc as our documentation solution. If need be, we can come up with a different solution (as part of my experimentation with monaco) for typing and docs. |
Matches the same fix already applied on #795 and #796: adds @sourceacademy/conductor to the yarn catalog and npmPreapprovedPackages, and switches midi's own package.json to "catalog:" instead of a literal "^0.7.0" pin, so this PR doesn't leave midi on the old convention once #795 lands. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
From what I can understand, it looks like the changes to I think having a unified format for how bundles are written is important: it will make any kind of loading implementation simpler. I intend to create a way to test bundles after compilation (right now Vitest does it pre-compilation), it would be a lot easier if I could always count on each bundle exporting the same thing |
…ave_to_note Addresses CodeRabbit's review comment on PR #796 (surfaced there via shared branch history with sound, but the finding is midi's own): add_octave_to_note's inline regex accepted note spellings that noteToValues/parseNoteWithOctave reject elsewhere (B#, E#, Cb, Fb - accidentals that don't exist for those note names), and let lowercase input escape unnormalized through a type assertion. Now validates via parseNoteWithOctave (rejecting digits upfront, since that function alone would accept an octave already being present) and reconstructs using the normalized note name, preserving the original accidental spelling exactly as given. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4 participants
Description
Fixes #775
Migrates the
midimodule to be Conductor-ready.midiis a dependency of bothsoundandstereo_sound, which import several of its functions (midi_note_to_frequency,letter_name_to_midi_note,letter_name_to_frequency) directly as plain TypeScript, and re-export them as part of their own Source-facing APIs. Rewritingmidi's functions to require anIDataHandler(the waybinary_tree/repeatdo) would have broken both of those call sites and re-exports immediately, so this migration keeps a pure, evaluator-free implementation available for cross-bundle TS consumption:functions.ts/scales.ts/utils.ts/types.ts— pure logic.scales.tsstill builds js-slang-style lists viapair/List, same as before.conductorAdapters.ts— new, undecorated helper used only by the plugin: converts a js-slang scale list into a real Conductor list. Kept out ofindex.tsdeliberately — a test file that importsindex.tsdirectly hits a decorator syntax error under vitest's transform (it doesn't apply the same decorator settingstscdoes), so anything meant to be unit-tested needs to live somewhere undecorated.index.ts— now aBaseModulePluginsubclass wrapping the pure functions.SHARP/FLAT/NATURALare plain string constants, not functions, soBaseModulePlugin.initialise()(which only registersexportedNamesthat are functions) can't pick them up — they're pushed ontothis.exportsdirectly in the constructor instead.sound/stereo_sound: only their imports of@sourceacademy/bundle-midichanged, to@sourceacademy/bundle-midi/functions(the bundle root now exports the Conductor plugin instead of plain functions). No other changes to either bundle — their own Conductor migration is a separate, larger effort (they're ~90% duplicated implementations of each other; deferred pending a decision on whether to merge them).Includes the same
__bindExportedMethods()workaroundrepeat/rune/binary_treeuse for the unbound-method issue inBaseModulePlugin.initialise()(source-academy/conductor#41, merged).Testing
yarn workspace @sourceacademy/bundle-midi run tsc/lint/test(29/29) /build— all passyarn workspace @sourceacademy/bundle-sound run tsc/test(28/28) — pass with the updated import pathyarn workspace @sourceacademy/bundle-stereo_sound run tsc/test(18/18) — pass with the updated import path