npm-upgrade (webpack 5)

[rvilarl]

Npm packages updated

[sschmidTU]

Cross-blob 3 can't be used with require, so at the very least this would necessitate merging the generateImages script to be an ESModule. We also need to check other effects of these upgrades, for example by running the visual regression tests (which needs generateImages).

[rvilarl]

Simon how should I run the visual regression test?

[sschmidTU]

As I said, you can't run it on this PR currently, because it needs generateImages_browserless.js, which doesn't work with cross-blob 3, because it's an ESModule now, so you can't require it. So it looks like the generateImages script needs to be migrated to be an ESModule and use imports instead of requires.

[sschmidTU]

Here's how you normally do it:
https://github.com/opensheetmusicdisplay/opensheetmusicdisplay/wiki/Testing

[rvilarl]

Migrated!

Found 117 current and 117 blessed png files (not tested if valid). Continuing.\n
Running 117 tests with threshold 0.0001 (nproc=8)...
Progress : [########################################] 100%
Results stored in ./visual_regression/diff/results.txt
All images with a difference over threshold, 0.0001, are
available in ./visual_regression/diff, sorted by perceptual hash.

Success - All diffs under threshold!

[sschmidTU]

Migrated!

Awesome, thank you very much! I'll take a look soon.

[sschmidTU]

@rvilarl The Create PDF function (button) in the demo is broken. (you changed the import)
console:

Uncaught TypeError: _node_modules_svg2pdf_js_dist_svg2pdf_umd_min__WEBPACK_IMPORTED_MODULE_3__ is not a function
    at createPdf (demo.js:899)
    at HTMLDivElement.printPdfBtn.onclick (demo.js:412)

line in the built demo (npm start):
_node_modules_svg2pdf_js_dist_svg2pdf_umd_min__WEBPACK_IMPORTED_MODULE_3__(svgElement, pdf, {

[sschmidTU]

Using svg2pdf.svg2pdf(svgElement, pdf, { in index.js:870 at least downloads a PDF, but returns an empty pdf and throws a new error in jspdf:
jspdf.min.js:40 Uncaught (in promise) TypeError: Cannot redefine property: sx

[sschmidTU]

@rvilarl I fixed the pdf function, we can even remove one deprecated import now.
Please apply this patch:
pdf_patch.txt

[rvilarl]

@sschmidTU how do I apply the patch ?

[sschmidTU]

@rvilarl git apply patchfile

[rvilarl]

@sschmidTU Done

[sschmidTU]

@rvilarl npm test isn't working for me locally, is it for you?
karma-server throws errors, starting with:

[karma-server]: Error during file loading or preprocessing
TypeError [ERR_INVALID_ARG_TYPE]: The "data" argument must be of type string or an instance of Buffer, TypedArray, or DataView. Received undefined
    at new NodeError (node:internal/errors:371:5)
    at Hash.update (node:internal/crypto/hash:105:11)
    at Object.sha1 (/Users/simon/OSMD/opensheetmusicdisplay-working-copy/node_modules/karma/lib/utils/crypto-utils.js:9:8)
    at runProcessors (/Users/simon/OSMD/opensheetmusicdisplay-working-copy/node_modules/karma/lib/preprocessor.js:70:26)

I upgraded to node 16, didn't fix it.

[rvilarl]

@sschmidTU yes it works:

Finished in 7.976 secs / 5.984 secs @ 16:07:47 GMT+0100 (hora estándar de Europa central)

SUMMARY:
✔ 170 tests completed
ℹ 1 test skipped

[rvilarl]

@sschmidTU I suggest that you delete the ./node_modules directory and call npm install.

[sschmidTU]

@rvilarl I did that twice now, it still doesn't work. Strange.

[rvilarl]

@sschmidTU how can we compare our environments?
ng --version returns:

Angular CLI: 13.0.3
Node: 14.18.1
Package Manager: npm 7.20.3
OS: linux x64

Angular: undefined
... 

Package                      Version
------------------------------------------------------
@angular-devkit/architect    0.1300.3 (cli-only)
@angular-devkit/core         13.0.3 (cli-only)
@angular-devkit/schematics   13.0.3 (cli-only)
@schematics/angular          13.0.3 (cli-only)
typescript                   4.5.2
webpack                      5.64.4

[sschmidTU]

I don't have ng, where is that from?

ng --version
bash: ng: command not found

I'm using:
Node: 16.13.0
Package Manager: npm 8.1.0
OS: MacOS

I doubt that Node 14 will make it work.
I also tried it with Node 12 before I upgraded.

[sschmidTU]

Still same error with your node and npm versions installed.

[sschmidTU]

Okay, I made it run with some hacks, seems to be a problem with karma-webpack 5 and MacOS.
It looks like karma-webpack 5 is not mature yet. You need it for Webpack 5 though.

In node_modules_karma-webpack/lib/karma-webpack/preprocessor.js,
I had to replace line 96 with this:
return bundleContent;

And in node_modules_karma-webpack/lib/webpack/plugin.js, I had to replace line 18 with this:
webpackFileObj.name.replace("Users", "../Users")
There is a ../ missing from the path to make it valid.
edit: plugin.js is fixed on karma-webpack master, it's just not released yet. preprocessor isn't fixed though.

I guess I'll have to open some issues at karma-webpack...

edit: both karma files seem fixed on their master. They just need to make a new release.

Related issues:
codymikol/karma-webpack#452
codymikol/karma-webpack#453

[sschmidTU]

The karma-webpack issue is fixed on their master, just not released yet.
I opened an issue about it here:
codymikol/karma-webpack#518

In the worst case, we could add a KarmaWebpackPatch system like the previous VexFlowPatch one and just copy the fixed files into node_modules/karma-webpack in npm's prebuild.

[rvilarl]

Very good! I am happy that you found it! So perhaps we wait for the fix to come and then merge this one.
Are you happy with the changes? Should I look into something else?

[sschmidTU]

The changes in this PR look good now except for the karma-webpack issue, I'll merge it once that is fixed. (If they don't release a new version in a few days, I'll create the KarmaWebpackPatch prebuild system)

[rvilarl]

@sschmidTU I just rebased the latest from development. It does not seem that karma-webpack is planning to release in short term. Should we go for the patch?

[sschmidTU]

It's not about visual tests (they don't run on Windows), but about npm test.
Error: EINVAL: invalid argument, mkdir 'C:\Users\Simon\AppData\Local\Temp\_karma_webpack_82360\D:\code\osmd\opensheetmusicdisplay\dist\src'

Maybe this is the problem:
codymikol/karma-webpack#433

Maybe you don't have different partitions on your hard drive.

[rvilarl]

I will give it a try... yes I get the same error. Is windows a problem? if visual regression tests do not run there, it is not really an adequated developing platform, is it?

[sschmidTU]

Yes, it's a problem. Visual Regression tests run on Windows using WSL.
The reason that they don't run without it is that they are written as a Unix shell script. They could also be rewritten to be in nodejs / npm.

[rvilarl]

In C:/ it runs well. I believe that this is a reasonalbe limitation that we could state in the README for a while until it is fixed. Don´t you agree?

[sschmidTU]

No. Partitions and drive letters aren't exactly a new concept.

[rvilarl]

OK let us try to progress on another front :) Can you help me with the VexFlowPatch issues that I have opened in VewFlow ?

[sschmidTU]

Yes, when I have the time.

[sschmidTU]

Commented on all the issues.
https://github.com/0xfe/vexflow/issues?q=is%3Aissue+OSMD+VexflowPatch

[sschmidTU]

@rvilarl I've decided to merge this to develop soon. I'll try to patch in correct paths for Windows in karma-webpack, but if it's too much work, I'll just accept the limitation that on Windows, npm test has to be done on the C:\ partition.
(this is on the feat/webpack5 branch currently)

[sschmidTU]

File paths on Windows fixed in 5fd2b5a and 75777bd.
Npm test continues working on Linux. MacOS should be the same.

So, now this can be finally merged to develop from feat/webpack5.

@rvilarl For rebasing your PRs etc. you should probably wait until this and the next OSMD Release which is coming soon.

[rvilarl]

@sschmidTU what is coming in the next release? I will make a prerelease branch in Vexflow soon and I am also looking on how to fix the Beam issues.

[sschmidTU]

@rvilarl Just the commits on develop since the last release, roughly this:

1.4.0 (2022-01-07 Preview)

Bug Fixes

• Cursor: Fix follow for multiple cursors, can set cursor.cursorOptions.follow for each (#1111) (37f9002)
• Grace Notes: Don't draw multiple grace not slashes for a set of grace notes (#1107) (89394db)
• GroupBrackets: Don't draw if only one instrument visible (b72ef4e)
• Positioning: Fix ghost notes only created for first few notes in measure, fixing cross stave positioning (#1062) (0507917)

• Note overlaps: fix notes overlapping / not staggering sometimes (this wasn't caught by commitizen-changelog)

Features

• ChordSymbols: add EngravingRules.DefaultColorChordSymbol (#1106) (7f00a9b)
• Note: Store TransposedPitch (for API access) (72633da)
• Slurs: GraphicalSlurs save their SVGElement (but not GraphicalTies) (5686daa)
• Transposing: Able to transpose single instrument (#1115) independently of other instruments (e997df9), closes #1116

[sschmidTU]

Turns out the npm test error in Windows comes from webpack, not directly from karma-webpack,
and it looks like it's not simple to fix, see PR #1119.

So we'll have to give up windows multiple drive support for now and just merge #1119,
this is the easiest option,
as it works on MacOS (with patch) and Linux now.

This is merged now by #1119.

[sschmidTU]

Webpack server hot reload for changes in node_modules/ is broken as well... (including hot reload for vexflowpatch)
@rvilarl what is your webpack.local.js configuration? can you please share it? you added it to package.json, but didn't provide it.

[rvilarl]

@sschmidTU where should I find my webpack.local.js ?

[sschmidTU]

@rvilarl i don't know, you added it with a npm start:local command in this PR. we can take it out again if it's not necessary.
https://github.com/opensheetmusicdisplay/opensheetmusicdisplay/blob/develop/package.json

[rvilarl]

@sschmidTU I do not think that I introduced that. It was already there in version 0.9.0