search

0xfe / vexflow

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

9b12482424dd0fc36846b9182519d3289a5e75bc: "qunit:files" (qunit) task failed. #1274

Closed h-sug1no closed 2 years ago

h-sug1no commented 2 years ago 
git log --merges

commit 9b12482424dd0fc36846b9182519d3289a5e75bc (HEAD, origin/master, origin/HEAD, master)
Merge: a2570076 c3b67d4b
Author: Mohit Cheppudira <0xfe@users.noreply.github.com>
Date:   Sun Dec 26 07:32:30 2021 -0500

    Merge pull request #1260 from rvilarl/easyscoreMutedHarmonySlashGhost2

    Easyscore support for muted, harmony, slash and ghost notes II (ghost)

commit a2570076497367ccba8ddc8a9ccdb299a882c754
Merge: 8cd3b7ea 0a8eec23
Author: Mohit Cheppudira <0xfe@users.noreply.github.com>
Date:   Sun Dec 26 07:17:12 2021 -0500

    Merge pull request #1268 from h-sug1no/vrtest-puppeteer-backend-2

    generate_images: add puppeteer(pptr) backend

git checkout . git checkout a2570076497367ccba8ddc8a9ccdb299a882c754 git clean -dfx && npm install && npm run lint && npm run qunit

OK: as follows:

...
Running "qunit:files" (qunit) task
Testing tests/flow-headless-browser.html
...
.........................................................................................................................................................................................................................................................................................................................OK
>> 1615 tests completed with 0 failed, 0 skipped, and 0 todo. 
>> 7194 assertions (in 16930ms), passed: 7194, failed: 0

Done.

git checkout . git checkout 9b12482424dd0fc36846b9182519d3289a5e75bc git clean -dfx && npm install && npm run lint && npm run qunit

NG: as follows:

...
Running "qunit:files" (qunit) task
Testing tests/flow-headless-browser.html FF
>> global failure
>> Message: Script error.
>> Actual: null
>> Expected: undefined
>> :0

>> global failure
>> Message: Uncaught ReferenceError: Vex is not defined
>> Actual: null
>> Expected: undefined
>> file:///mnt/sdb/users/sug1no/work3/github/h-sug1no/vexflow.d/vexflow/tests/flow-headless-browser.html:12

Warning: 2 tests completed with 2 failed, 0 skipped, and 0 todo. 
2 assertions (in 8ms), passed: 0, failed: 2 Use --force to continue.

Aborted due to warnings.

load file:///mnt/sdb/users/sug1no/work3/github/h-sug1no/vexflow.d/vexflow/tests/flow-headless-browser.html with chrome, the following errors are shown in console:

vexflow-debug-with-tests.js:19273 Uncaught ReferenceError: Cannot access 'Note' before initialization
    at Module.Note (vexflow-debug-with-tests.js:19273)
    at Object../src/stemmablenote.ts (vexflow-debug-with-tests.js:24957)
    at __webpack_require__ (vexflow-debug-with-tests.js:48373)
    at Object../src/tabnote.ts (vexflow-debug-with-tests.js:27719)
    at __webpack_require__ (vexflow-debug-with-tests.js:48373)
    at Object../src/dot.ts (vexflow-debug-with-tests.js:4161)
    at __webpack_require__ (vexflow-debug-with-tests.js:48373)
    at Object../src/note.ts (vexflow-debug-with-tests.js:19276)
    at __webpack_require__ (vexflow-debug-with-tests.js:48373)
    at Object../src/notehead.ts (vexflow-debug-with-tests.js:19815)
flow-headless-browser.html:12 Uncaught ReferenceError: Vex is not defined
    at flow-headless-browser.html:12


@rvilarl san: @0xfe san:

dependency problem?

rvilarl commented 2 years ago

I am looking into that as well. Yes it is a circular dependency problem. @ronyeh could you help us? You already had that.

ronyeh commented 2 years ago

If you turn on the circular dependency flag in the gruntfile, it prints out a list of possible circular dependencies to investigate.

ronyeh commented 2 years ago

I have a little bit of time at the moment. :-)

ronyeh commented 2 years ago

Still debugging.... But here is some debug output re: circular dependencies:

$> VEX_DEBUG_CIRCULAR_DEPENDENCIES=true   grunt test

WARNING in Circular dependency detected:
src/accidental.ts -> src/gracenote.ts -> src/stavenote.ts -> src/notehead.ts -> src/note.ts -> src/accidental.ts

src/articulation.ts -> src/gracenote.ts -> src/stavenote.ts -> src/notehead.ts -> src/note.ts -> src/accidental.ts -> src/gracenotegroup.ts -> src/beam.ts -> src/tuplet.ts -> src/formatter.ts -> src/modifiercontext.ts -> src/articulation.ts

src/beam.ts -> src/stavenote.ts -> src/notehead.ts -> src/note.ts -> src/accidental.ts -> src/gracenotegroup.ts -> src/beam.ts

src/dot.ts -> src/gracenote.ts -> src/stavenote.ts -> src/notehead.ts -> src/note.ts -> src/accidental.ts -> src/gracenotegroup.ts -> src/beam.ts -> src/tabnote.ts -> src/dot.ts

src/factory.ts -> src/system.ts -> src/factory.ts

src/formatter.ts -> src/beam.ts -> src/stavenote.ts -> src/notehead.ts -> src/note.ts -> src/accidental.ts -> src/gracenotegroup.ts -> src/formatter.ts

src/glyph.ts -> src/tables.ts -> src/glyph.ts

src/gracenote.ts -> src/stavenote.ts -> src/notehead.ts -> src/note.ts -> src/accidental.ts -> src/gracenote.ts

src/gracenotegroup.ts -> src/beam.ts -> src/stavenote.ts -> src/notehead.ts -> src/note.ts -> src/accidental.ts -> src/gracenotegroup.ts

src/modifiercontext.ts -> src/accidental.ts -> src/gracenotegroup.ts -> src/beam.ts -> src/tuplet.ts -> src/formatter.ts -> src/modifiercontext.ts

src/note.ts -> src/accidental.ts -> src/gracenote.ts -> src/stavenote.ts -> src/notehead.ts -> src/note.ts

src/notehead.ts -> src/note.ts -> src/accidental.ts -> src/gracenote.ts -> src/stavenote.ts -> src/notehead.ts

src/notesubgroup.ts -> src/formatter.ts -> src/modifiercontext.ts -> src/notesubgroup.ts

src/ornament.ts -> src/tabnote.ts -> src/dot.ts -> src/gracenote.ts -> src/stavenote.ts -> src/notehead.ts -> src/note.ts -> src/accidental.ts -> src/gracenotegroup.ts -> src/beam.ts -> src/tuplet.ts -> src/formatter.ts -> src/modifiercontext.ts -> src/ornament.ts

src/stave.ts -> src/stavetext.ts -> src/textnote.ts -> src/note.ts -> src/accidental.ts -> src/gracenotegroup.ts -> src/beam.ts -> src/tuplet.ts -> src/formatter.ts -> src/stave.ts

src/stavenote.ts -> src/notehead.ts -> src/note.ts -> src/accidental.ts -> src/gracenote.ts -> src/stavenote.ts

src/stavetext.ts -> src/textnote.ts -> src/note.ts -> src/accidental.ts -> src/gracenotegroup.ts -> src/beam.ts -> src/tuplet.ts -> src/formatter.ts -> src/stave.ts -> src/stavetext.ts

src/stemmablenote.ts -> src/note.ts -> src/accidental.ts -> src/gracenote.ts -> src/stavenote.ts -> src/stemmablenote.ts

src/stringnumber.ts -> src/stavenote.ts -> src/notehead.ts -> src/note.ts -> src/accidental.ts -> src/gracenotegroup.ts -> src/beam.ts -> src/tuplet.ts -> src/formatter.ts -> src/modifiercontext.ts -> src/stringnumber.ts

src/strokes.ts -> src/note.ts -> src/accidental.ts -> src/gracenotegroup.ts -> src/beam.ts -> src/tuplet.ts -> src/formatter.ts -> src/modifiercontext.ts -> src/strokes.ts

src/system.ts -> src/factory.ts -> src/system.ts

src/tables.ts -> src/glyph.ts -> src/tables.ts

src/tabnote.ts -> src/dot.ts -> src/gracenote.ts -> src/stavenote.ts -> src/notehead.ts -> src/note.ts -> src/accidental.ts -> src/gracenotegroup.ts -> src/beam.ts -> src/tabnote.ts

src/textnote.ts -> src/note.ts -> src/accidental.ts -> src/gracenotegroup.ts -> src/beam.ts -> src/tuplet.ts -> src/formatter.ts -> src/stave.ts -> src/stavetext.ts -> src/textnote.ts

src/tuplet.ts -> src/formatter.ts -> src/beam.ts -> src/tuplet.ts

Uncaught ReferenceError: Cannot access 'Note' before initialization
    at Module.Note (vexflow-debug-with-tests.js:19273)
    at Object../src/stemmablenote.ts (vexflow-debug-with-tests.js:24957)
    at __webpack_require__ (vexflow-debug-with-tests.js:48373)
    at Object../src/tabnote.ts (vexflow-debug-with-tests.js:27719)
    at __webpack_require__ (vexflow-debug-with-tests.js:48373)
    at Object../src/dot.ts (vexflow-debug-with-tests.js:4161)
    at __webpack_require__ (vexflow-debug-with-tests.js:48373)
    at Object../src/note.ts (vexflow-debug-with-tests.js:19276)
    at __webpack_require__ (vexflow-debug-with-tests.js:48373)
    at Object../src/notehead.ts (vexflow-debug-with-tests.js:19815)

This one appears 4 times: src/note.ts -> src/accidental.ts -> src/gracenote.ts -> src/stavenote.ts

Since the error happens in StemmableNote, it seems like this circular chain might be one we should try to break up:

src/stemmablenote.ts -> src/note.ts -> src/accidental.ts -> src/gracenote.ts -> src/stavenote.ts -> src/stemmablenote.ts
ronyeh commented 2 years ago

Another thing we should try to do in general, is avoid importing the index.ts files. For example:

In annotation_tests.js: AVOID: import { ModifierPosition } from '../src';

DO THIS INSTEAD: import { ModifierPosition } from '../src/modifier';

In bend_tests.js: AVOID: import { Font } from '../src'; DO THIS INSTEAD: import { Font } from '../src/font';

Importing the class files directly can help avoid circular dependencies, and in the future it might help bundlers with tree shaking.

ronyeh commented 2 years ago

Hi @rvilarl

I submitted a PR with an alternate solution: https://github.com/0xfe/vexflow/pull/1276

Let me know what you think! The basic idea is to completely avoid using isTabNote() and isStaveNote() in dot.ts, by moving the custom functionality out of the Dot class and into TabNote and StaveNote. This means that Dot no longer depends on TabNote and StaveNote.

ronyeh commented 2 years ago

@0xfe @rvilarl

I support Rodrigo's fix https://github.com/0xfe/vexflow/pull/1275 because it's the simplest patch at the moment, and it fixes the build. (I assume there are no visual diffs between #1275 and the previous non-broken build?).

As for the larger topic of circular dependencies, I think we could revisit the type guard methods of the type isXXX(), e.g., isStaveNote(), isTabNote(), etc.

I think there are two possible solutions:

  1. Element is already an abstract base class. We could add all the type guard methods to the Element class. For example, e.isStaveNote() and e.isTabNote(). By default, all the implementations return false. Only the correct subclass will override the relevant method and return true. This breaks the circular dependency chain, because it does not rely on the instanceof keyword.
  2. Refactor some areas of the code where we do type checking and then do slightly different things based on the element type. Instead, call a method like .doSomething(), and then different classes will have different implementations of .doSomething().

I think option 1 is simpler, but option 2 might improve the readability of the code.

Perhaps we can start with option 1, and then move toward option 2 over time?

0xfe commented 2 years ago

Thanks for this folks -- I'm merging #1275 because it's simpler.

Ron, not a huge fan of putting typeguards in the Element class. I'd rather use a simple getCategory check instead for sure. Still, the general issue with circular dependencies is something we need to figure out a coherent strategy for. (I'm still unclear why it broke this time.)