Midi Import via midiToSequenceProto broken

[j0hnm4r5]

This issue actually looks to be a problem with the midiconvert from Tone.js, but I ran into it here.

Issue

Importing a midi file using mm.midiToSequenceProto(midiFile) fails with this error:

Midi.js:86 Uncaught TypeError: Decoder is not a function
    at Midi.decode (Midi.js:86)
    at Object.parse (MidiConvert.js:43)
    at Object.midiToSequenceProto (midi_io.ts:39)
    at FileReader.<anonymous> (....js:14)

Solution

Midi.js within the package midiconvert calls Decoder(bytes) from the midi-file-parser package (expecting it to be a function), except midi-file-parser exports that function within an object.

Changing const midiData = Decoder(bytes) to const midiData = Decoder.default(bytes) within node_modules/midiconvert/src/Midi.js fixes the issue.

[adarob]

Can you please file a bug against midiconvert and link it to this one so we'll know when to update.

FYI, @tambien

[j0hnm4r5]

Yep, sorry — should have done that originally.

Here it is: Tonejs/Midi#70

[j0hnm4r5]

This seems weird — midiconvert redirects to @tonejs/midi (it doesn't have its own git repo) and it hasn't been updated in over a year. @tonejs/midi has the same file and folder structure as midiconvert, but Midi.js doesn't reference the decoder in the same way (making me believe this bug has already been fixed).

I'm still waiting on hard confirmation from the linked issue, but I'm pretty sure midiconvert was wrapped into @tonejs/midi (here: Tonejs/Midi@2605351#diff-f70adec9761ae64004e9d20a1a44f71e), and magenta-js is still using midiconvert when it should be using @tonejs/midi?

[tambien]

@j0hnm4r5 yeah i did a major refactor a few months ago and released it under a new name. i changed the encoder/decoder and how it is being imported, so this issue might not be relevant any more to the latest version.

[mattetti]

It looks like we need a new issue to upgrade the tone.js dependency (currently set to "tone": "^0.12.80" when the latest version is 13.4.9 (the currently locked version is more than a year old)

https://www.npmjs.com/package/tone

[notwaldorf]

Unfortunately last time I tried to upgrade to tone 13.x, it broke all the Players 😭. @tambien, did you ever figure out why that was?

[tambien]

The issue with the upgrade seems to be that the PolySynth sustains and glitches out for the first note when it's played. This seems to be because the synth is created directly before it's used. If you instead create it in the constructor initialization, this seems to fix the issue. i'll send a PR with some suggested changes hopefully today.

[notwaldorf]

Good news: we've updated Tone to 13.x! I'm going to optimistically close this as "fixed", but if it's still borked, please re-open it!