Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migration from 3.0.9 to 4.0 #1200

Closed
ronyeh opened this issue Oct 27, 2021 · 32 comments · Fixed by #1214
Closed

Migration from 3.0.9 to 4.0 #1200

ronyeh opened this issue Oct 27, 2021 · 32 comments · Fixed by #1214

Comments

@ronyeh
Copy link
Collaborator

ronyeh commented Oct 27, 2021

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
Copy link
Collaborator Author

ronyeh commented Oct 29, 2021

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
Copy link
Owner

0xfe commented Oct 30, 2021

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
Copy link
Collaborator

AaronDavidNewman commented Oct 30, 2021

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
Copy link
Collaborator Author

ronyeh commented Oct 30, 2021

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
Copy link
Collaborator

rvilarl commented Oct 30, 2021

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
Copy link
Collaborator

rvilarl commented Oct 30, 2021

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
Copy link
Collaborator

rvilarl commented Oct 30, 2021

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

@AaronDavidNewman
Copy link
Collaborator

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

@rvilarl
Copy link
Collaborator

rvilarl commented Oct 31, 2021

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
Copy link
Collaborator

rvilarl commented Oct 31, 2021

@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
Copy link
Collaborator

rvilarl commented Oct 31, 2021

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

@rvilarl rvilarl mentioned this issue Oct 31, 2021
@rvilarl
Copy link
Collaborator

rvilarl commented Oct 31, 2021

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

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

@ronyeh
Copy link
Collaborator Author

ronyeh commented Oct 31, 2021

Oh thanks for your help! My current commit that I've tested is b1473d9 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
Copy link
Collaborator

rvilarl commented Oct 31, 2021

yes, 100% TS via npm install / npm link

@rvilarl
Copy link
Collaborator

rvilarl commented Oct 31, 2021

Same output with b1473d9

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

@ronyeh
Copy link
Collaborator Author

ronyeh commented Oct 31, 2021

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
Copy link
Collaborator

rvilarl commented Oct 31, 2021

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

@ronyeh
Copy link
Collaborator Author

ronyeh commented Oct 31, 2021

I found a related question and answer on:

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

@rvilarl
Copy link
Collaborator

rvilarl commented Oct 31, 2021

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
Copy link
Collaborator Author

ronyeh commented Oct 31, 2021

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
Copy link
Collaborator Author

ronyeh commented Oct 31, 2021

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
Copy link
Collaborator

rvilarl commented Oct 31, 2021 via email

@ronyeh
Copy link
Collaborator Author

ronyeh commented Oct 31, 2021 via email

@ronyeh
Copy link
Collaborator Author

ronyeh commented Oct 31, 2021 via email

@rvilarl
Copy link
Collaborator

rvilarl commented Oct 31, 2021

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

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

@ronyeh
Copy link
Collaborator Author

ronyeh commented Nov 1, 2021

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
Copy link
Collaborator Author

ronyeh commented Nov 4, 2021

See this thread for first steps on migrating your project to 4.0.0:
#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
Copy link
Collaborator

rvilarl commented Nov 4, 2021

@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
Copy link
Collaborator

rvilarl commented Nov 5, 2021

@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
Copy link
Collaborator Author

ronyeh commented Nov 6, 2021

I successfully migrated VexTab to VexFlow 4.0.0.

This is the patch for the migration of VexTab: 0xfe/vextab#137

I had to modify VexFlow a bit to restore some backwards compatibility. I also added some new accessors:
ronyeh/vexflow@e5747ba

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
Copy link
Collaborator

rvilarl commented Nov 6, 2021

@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
Copy link
Collaborator

rvilarl commented Nov 6, 2021

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

@ronyeh ronyeh linked a pull request Nov 12, 2021 that will close this issue
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 a pull request may close this issue.

4 participants