0xfe / vexflow

A JavaScript library for rendering music notation and guitar tablature.
http://www.vexflow.com
Other
3.91k stars 662 forks source link

Migration from 3.0.9 to 4.0 #1200

Closed ronyeh closed 2 years ago

ronyeh commented 3 years ago

This meta issue tracks any known stumbling blocks when migrating a project from 3.0.9 to 4.0.

These stumbling blocks can be addressed in our CHANGELOG file.

One idea is to use npm link to point the VexTab project at VexFlow 4.0, and then see what errors pop up. You can do the same with your own project, and list some of the errors here.

We can promote some to independent issues, if they are bad enough to warrant a bug fix.

ronyeh commented 3 years ago

I've been working on two independent example repos that "npm install vexflow" so I can check for integration / migration issues. The first repo is in JS, and the second repo is in TS.

I use npm link / npm link vexflow to point node_modules/vexflow/ at my local 4.0.0 repo. I've gotten some things to work, but also ran into some issues. For example:

$> node index.js
internal/process/esm_loader.js:74
    internalBinding('errors').triggerUncaughtException(
                              ^
TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for vexflow/src/index.ts
    at Loader.defaultGetFormat [as _getFormat] (internal/modules/esm/get_format.js:65:15)
    at Loader.getFormat (internal/modules/esm/loader.js:101:42)

This error makes sense, because my node script is in JS but we're pointing it at TS sources. Our package.json specifies the main entry point to be index.ts. It should probably be the minimized production build: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#including-declarations-in-your-npm-package

Additionally, we'll need to build a *.d.ts declarations file.

import Vex from "vexflow";
console.log("VexFlow Build: " + Vex.Flow.BUILD);

Here's what Tone.js does: https://github.com/Tonejs/Tone.js/blob/5c15c9517da2de9450906429c2916f48d8942d05/package.json#L5-L8

I got my example repo to work by changing our package.json to point to webpacked production JS file:

  "main": "build/vexflow.js",
  "main": "src/index.ts", <== DELETE THIS LINE

However, I'm not an expert in all the different fields of package.json. Anybody else have a clue? Otherwise I'm inclined to copy other projects' approaches :-). The Tone.js approach seems OK. They are a popular MIT licensed TS audio/music project, so I'm sure they don't mind if we learn from their package.json and webpack settings.

The relevant package.json fields might be:

"browser": "build/Tone.js",
"main": "build/esm/index.js",
"module": "build/esm/index.js",
"type": "module",

Once I'm done with my two demo repos, I'll post the link here.

0xfe commented 3 years ago

Ron, can you try it with https://github.com/0xfe/vextab please? I have instructions there around using npm link(see README) for developing with both VexFlow and VexTab at the same time.

Re: fields of package.json -- I'm no expert either, I really just used what worked without searching for the best way. I think if you have good representative TS packages to model off of, that would be great. Just be mindful of emerging complexity in the build pipeline -- it makes debugging really hard. In general, I prefer simple imperfect workflows to ideal workflows.

AaronDavidNewman commented 3 years ago

Edit: correct the file name with the issue.

I think there is an issue with our current build that we may want to fix before we release. It looks like the symbol Vex in vf_prefix_tests.ts:

image

is only created in releases/vexflow-tests.js, which I assume was the output of the build at an earlier time. If you don't have anything in your releases directory, the build will fail with unknown symbol 'Vex'.

I think it will work just to replace that line with:

const VF = Flow;
ronyeh commented 3 years ago

I think there is an issue with our current build that we may want to fix before we release. It looks like the symbol Vex in vf_prefix_tests.ts:

This issue is fixed in my branch. With declare let Vex: any;:

https://github.com/ronyeh/vexflow/blob/e1b596ec17b76def76bdbb19835179fc6acd3d26/tests/vf_prefix_tests.ts#L93-L97

That test case is intentionally accessing Vex.Flow so we can compare it against direct access to the classes in TypeScript. I added those test cases a while back, and perhaps a bad merge/rebase killed a line.

We are just verifying that the global Vex.Flow exists, and things like Vex.Flow.Accidental are equivalent to Accidental.

rvilarl commented 3 years ago

Starting the migration with PianoPlay...

Issues: 1) imports without path -> Solution: Path added image 2) legacy fonts in src directory -> Solution: Legacy moved to tools/fonts/ 3) @bravura, @gonville, @petaluma ... -> Workaround: change to './fonts/loadStatic'

rvilarl commented 3 years ago

Migration done! TS migrated version https://michaelecke.com/pianoplayts/#/home Original (1.2.93) https://michaelecke.com/pianoplay/#/home

You can open any MusicXML file

They look quite different indeed

Captura de pantalla de 2021-10-31 00-33-06

Captura de pantalla de 2021-10-31 00-32-59

rvilarl commented 3 years ago

The notes are going beyond the staves :( @sschmidTU you know that I use VexFlow with OSMD, any tip?

AaronDavidNewman commented 3 years ago

This looks like an issue with clefNote. What are you using to compute the width - is it preCalculateMinTotalWidth?

rvilarl commented 3 years ago

OSMD uses the following:

      minStaffEntriesWidth = formatter.preCalculateMinTotalWidth(allVoices) / unitInPixels
      * this.rules.VoiceSpacingMultiplierVexflow
      + this.rules.VoiceSpacingAddendVexflow
      + maxStaffEntries * staffEntryFactor; // TODO use maxStaffEntriesPlusAccidentals here as well, adjust spacing

@sschmidTU this is the calculation, is not it?

rvilarl commented 3 years ago

@AaronDavidNewman @sschmidTU I made an error updating the web page and I left and early version of the TS migrated code. The latest one looks much better indeed!

Captura de pantalla de 2021-10-31 06-37-57

Old 1.2.93 version

Captura de pantalla de 2021-10-31 00-33-06

rvilarl commented 3 years ago

I woulld say that it looks also finally better than 1.2.93! This is great! I will try wit other scores.

rvilarl commented 3 years ago

@ronyeh I also tried with your branch in #1163 commit 50129fca5db4ca488d9bbd36fba7663f62651454 and I get the following errors (not many):

Captura de pantalla de 2021-10-31 09-18-06

ronyeh commented 3 years ago

Oh thanks for your help! My current commit that I've tested is b1473d90cc618a073af336bc4125e8510027222b on https://github.com/ronyeh/vexflow/tree/setFont

The @loadFonts bit was inspired by your tsconfig paths for @bravura, @gonville, @petaluma. That seems like the biggest issue to figure out. Is it a tsconfig problem?

I assume your project is 100% TS and you are compiling with vexflow (via npm install / npm link)?

rvilarl commented 3 years ago

yes, 100% TS via npm install / npm link

rvilarl commented 3 years ago

Same output with b1473d90cc618a073af336bc4125e8510027222b

Captura de pantalla de 2021-10-31 09-28-48

ronyeh commented 3 years ago

The FontFace API is part of the web standard, and on my vscode is available via TypeScript's lib.dom.d.ts. https://github.com/microsoft/TypeScript/blob/main/lib/lib.dom.d.ts

I didn't have to do anything for VSCode to recognize this type. For example interface HTMLElement extends Element is also defined in lib.dom.d.ts.

I wonder why your VSCode doesn't recognize that type?

rvilarl commented 3 years ago

I might be missing a npm install in my project? I say that because I have no problem compiling b1473d90cc618a073af336bc4125e8510027222b.

ronyeh commented 3 years ago

I found a related question and answer on:

Does that mean I can just add that typings file as a dependency for VexFlow?

rvilarl commented 3 years ago

I installed npm install --save @types/css-font-loading-module and I still get the problem :(

Does that mean I can just add that typings file as a dependency for VexFlow?

I do not know

ronyeh commented 3 years ago

Weird. But when you compile my branch, it compiles fine? If you ctrl+click (or alt+click on Mac) the name FontFace, does it bring you to lib.dom.d.ts?

Anyways, thanks for your help. I'll see if I can repro this.

ronyeh commented 3 years ago

Can you dig around and see if you can find your copy of lib.dom.d.ts? Where is it stored? Which distro / OS are you using?

Even though the TS compiler and VS Code should recognize FontFace natively, I don't want this to be a stumbling block for developers who want to use VexFlow.

Either we need to find an npm package (of typings) that will make this work seamlessly, or perhaps I can declare FontFace as a global any, and then just drop in an eslint-disable to squash the warnings, haha.

rvilarl commented 3 years ago

I will do.

I have also placed complete paths in imports on all the files #1209. This is also to avoid the problems with npm link, linux...

I use Fedora 34

ronyeh commented 3 years ago

I pushed one last update for tonight.

cbebfb03ae45baa2d036e7248a7d4adb01abad10

I just added a src/types/fontface.d.ts

Hopefully that will allow the types to work!

On Sun, Oct 31, 2021 at 3:16 AM Rodrigo Vilar @.***> wrote:

I will do.

I have also placed complete paths in imports on all the files in migration/4.0. This is also to avoid the problems with linux...

I use Fedora 34

El dom, 31 de oct de 2021 a las 03:13:03 AM, Ron B. Yeh @.***> escribió:

Can you dig around and see if you can find your copy of lib.dom.d.ts? Where is it stored? Which distro / OS are you using?

Even though the TS compiler and VS Code should recognize FontFace natively, I don't want this to be a stumbling block for developers who want to use VexFlow.

Either we need to find a npm package (of typings) that will make this work seamlessly, or perhaps I can declare FontFace as a global any, and then just drop in an eslint-disable to squash the warnings, haha.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/0xfe/vexflow/issues/1200#issuecomment-955671911, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AFOOLBMGSNNDBHJOAIU5YXLUJUJC7ANCNFSM5G2U3ECQ .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/0xfe/vexflow/issues/1200#issuecomment-955672367, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB2MCNZYXXJNEQEXOUSPM3UJUJPHANCNFSM5G2U3ECQ .

ronyeh commented 3 years ago

This is what the file looks like: https://github.com/ronyeh/vexflow/commit/cbebfb03ae45baa2d036e7248a7d4adb01abad10

https://github.com/ronyeh/vexflow/tree/setFont

Good night! Putting my laptop (and me) to sleep. :-)

On Sun, Oct 31, 2021 at 3:19 AM Ron Yeh @.***> wrote:

I pushed one last update for tonight.

cbebfb03ae45baa2d036e7248a7d4adb01abad10

I just added a src/types/fontface.d.ts

Hopefully that will allow the types to work!

On Sun, Oct 31, 2021 at 3:16 AM Rodrigo Vilar @.***> wrote:

I will do.

I have also placed complete paths in imports on all the files in migration/4.0. This is also to avoid the problems with linux...

I use Fedora 34

El dom, 31 de oct de 2021 a las 03:13:03 AM, Ron B. Yeh @.***> escribió:

Can you dig around and see if you can find your copy of lib.dom.d.ts? Where is it stored? Which distro / OS are you using?

Even though the TS compiler and VS Code should recognize FontFace natively, I don't want this to be a stumbling block for developers who want to use VexFlow.

Either we need to find a npm package (of typings) that will make this work seamlessly, or perhaps I can declare FontFace as a global any, and then just drop in an eslint-disable to squash the warnings, haha.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/0xfe/vexflow/issues/1200#issuecomment-955671911, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AFOOLBMGSNNDBHJOAIU5YXLUJUJC7ANCNFSM5G2U3ECQ .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/0xfe/vexflow/issues/1200#issuecomment-955672367, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB2MCNZYXXJNEQEXOUSPM3UJUJPHANCNFSM5G2U3ECQ .

rvilarl commented 3 years ago

Good night! Here the ouput, still an error but different.

Captura de pantalla de 2021-10-31 11-28-15

ronyeh commented 3 years ago

Just updating folks on this thread. Rodrigo and I are collaborating over chat at the moment. We have figured out the FontFace error. The API declaration was introduced in TypeScript 4.4, and he's running TS 4.3.5. So I put in @ts-ignore comments in the appropriate places to ask VS Code / TS compiler to ignore errors in that method for now.

I'm now trying to figure out our path alias issue.

ronyeh commented 3 years ago

See this thread for first steps on migrating your project to 4.0.0: https://github.com/0xfe/vexflow/pull/1214

Quick Start:

cd /path/to/your/project
npm uninstall vexflow
npm install https://vexflow-demo.surge.sh/4.0.0-alpha-1.tgz

Please respond with the errors you're seeing. I can help point you in the right direction.

Or if I'm away from keyboard, I'm sure Rodrigo can also help, since he seems to be intimately familiar with my code.... Also, we're 8 hrs offset from each other, so you'll almost have 24 hr tech support for this upgrade. One time special offer! ;-)

rvilarl commented 3 years ago

@sschmidTU Ron and I decided to give a try at vanilla OSMD migration without my app PianoPlay. These are the steps:

mkdir integration
cd integration
git clone https://github.com/ronyeh/vexflow
cd vexflow
git checkout 4.0.0-alpha
npm install 
npm run reference
cd ..
git clone https://github.com/opensheetmusicdisplay/opensheetmusicdisplay
cd opensheetmusicdisplay/

edit package.json: remove the line "prebuild": "ncp src/VexFlowPatch/src/ node_modules/vexflow/src/", remove the dependencies on @types/vexflow & vexflow

npm install

At this stage the compilation will fail because vexflow is missing.

sudo npm link ../vexflow/
npm run build

@ronyeh @sschmidTU I believe that the steps are now the right ones and I get a lot of errors (thoughts?):

Captura de pantalla de 2021-11-05 17-54-42

rvilarl commented 3 years ago

@sschmidTU can you try the mini guide above and provide some feedback/help? Am I missing something or doing something wrong? I am not progressing. :(

ronyeh commented 3 years ago

I successfully migrated VexTab to VexFlow 4.0.0.

This is the patch for the migration of VexTab: https://github.com/0xfe/vextab/pull/137

I had to modify VexFlow a bit to restore some backwards compatibility. I also added some new accessors: https://github.com/ronyeh/vexflow/commit/e5747baaa573d39a357b9767a61aa74f89781da2

I can cherry pick that commit and submit it as a separate PR if it makes it easier to merge.

Here is the process I used to migrate VexTab. It might help others with migrating their own projects to VexFlow 4.

git clone git@github.com:0xfe/vextab.git .

npm install

npm run start

    Everything works, because it is using VexFlow 3.0.8. View the tests at http://localhost:9005/

~~~TIME TO MIGRATE!~~~

delete node_modules/vexflow/

npm run start

    ERROR: vextab/src/div.js
    Unable to resolve path to module 'vexflow'  import/no-unresolved
    This is expected, because I just deleted vexflow :-)

npm install https://vexflow-demo.surge.sh/vexflow-4.0.0-alpha-2.tgz

    Alternatively, clone vexflow from https://github.com/ronyeh/vexflow
    Check out the `4.0.0-alpha` branch
    Build VexFlow 4 by running `grunt`
    Run `npm link` to register that branch. 
    Change back to the vextab/ directory.
    npm link vexflow

npm run start

There were some errors, but I was able to debug & fix them. Not too bad.

Here are the screenshots after the VexTab migration.

Playground

Tests

rvilarl commented 3 years ago

@sschmidTU can you try the mini guide above and provide some feedback/help? Am I missing something or doing something wrong? I am not progressing. :(

@ronyeh @sschmidTU the problem in the migration is that the imported vex can be used to instantiate objects (ie.: Vex.Flow.StaveNote): vfnote = new Vex.Flow.StaveNote(vfnoteStruct); but cannot be used in: 1) types/function parameters vfnote: Vex.Flow.StaveNote 2) return types in functions getNote(): Vex.Flow.StaveNote {... some, not all, can be solved with typeof

I have made a pul request to my own fork so that you can see the changes

https://github.com/rvilarl/opensheetmusicdisplay/pull/2/files

rvilarl commented 3 years ago

Several scores using the migration above in PianoPlay. @ronyeh this is using #1214 without modification, this is very good! We must find a way though to avoid the usage of any in the migration of OSMD Captura de pantalla de 2021-11-06 14-52-49 Captura de pantalla de 2021-11-06 14-59-20 Captura de pantalla de 2021-11-06 15-02-37