Closed ronyeh closed 2 years ago
I've looked a bit into JS imports in the past, and it seems like a bit of a mess to me, so I don't have a clear preference for now. I probably need to research it more. But I still don't understand why there is no standard way to export things in javascript so that you can import it everywhere. (And even for specific solutions, sometimes importing randomly fails.) For script tag and require, the browser or engine should take care of it. Though my ranting probably isn't too helpful ;)
Thanks for exploring the options! Sounds good to me so far. It does seem like things are trending toward these module type exports. Though I do also still see quite a few hobby developers who still like to use script tag imports for simple HTML/JS projects.
Thanks for the note! In my 4.0 branch, all of the options I mentioned are supported.
The only issue is that our project now defaults to Common JS style build product, and I have to hack it to produce the ES modules. Going forward (maybe in v5.0), I believe we should default everything to ESM, and then make CJS the special case.
These are the build output JS files in the current 4.0 alpha:
The vexflow/build/ folder contains all the regular CJS files which work with script tag and require:
vexflow-core-with-bravura.js
vexflow-core-with-gonville.js
vexflow-core-with-petaluma.js
vexflow-core.js
vexflow-debug-with-tests.js
vexflow-debug.js
vexflow-font-bravura.js
vexflow-font-custom.js
vexflow-font-gonville.js
vexflow-font-petaluma.js
vexflow.js
Inside the vexflow/build/esm/ folder are the ES modules:
package.json
vexflow-core-with-bravura.js
vexflow-core-with-gonville.js
vexflow-core-with-petaluma.js
vexflow-core.js
vexflow-debug-with-tests.js
vexflow-debug.js
vexflow-font-bravura.js
vexflow-font-custom.js
vexflow-font-gonville.js
vexflow-font-petaluma.js
vexflow.js
The esm folder has an extra package.json that tells client code to treat these JS files as ES modules:
{
"type": "module"
}
Each of the ESM files is just a copy of the regular CJS files, but with a small hack at the bottom that exports the necessary variables:
const Vex = globalThis['Vex'];
const Flow = Vex.Flow;
const { Accidental, Annotation, Articulation, BarNote, Beam, Bend, BoundingBox, BoundingBoxComputation, ChordSymbol, Clef, ClefNote, Crescendo, Curve, Dot, EasyScore, Element, Factory, Font, Formatter, Fraction, FretHandFinger, GhostNote, Glyph, GlyphNote, GraceNote, GraceNoteGroup, GraceTabNote, KeyManager, KeySignature, KeySigNote, Modifier, ModifierContext, MultiMeasureRest, Music, Note, NoteHead, NoteSubGroup, Ornament, Parser, PedalMarking, Registry, RenderContext, Renderer, RepeatNote, Stave, Barline, StaveConnector, StaveHairpin, StaveLine, StaveModifier, StaveNote, Repetition, StaveTempo, StaveText, StaveTie, Volta, Stem, StringNumber, Stroke, System, Tables, TabNote, TabSlide, TabStave, TabTie, TextBracket, TextDynamics, TextFormatter, TextNote, TickContext, TimeSignature, TimeSigNote, Tremolo, Tuning, Tuplet, Vibrato, VibratoBracket, Voice } = Vex.Flow;
export { Accidental, Annotation, Articulation, BarNote, Beam, Bend, BoundingBox, BoundingBoxComputation, ChordSymbol, Clef, ClefNote, Crescendo, Curve, Dot, EasyScore, Element, Factory, Font, Formatter, Fraction, FretHandFinger, GhostNote, Glyph, GlyphNote, GraceNote, GraceNoteGroup, GraceTabNote, KeyManager, KeySignature, KeySigNote, Modifier, ModifierContext, MultiMeasureRest, Music, Note, NoteHead, NoteSubGroup, Ornament, Parser, PedalMarking, Registry, RenderContext, Renderer, RepeatNote, Stave, Barline, StaveConnector, StaveHairpin, StaveLine, StaveModifier, StaveNote, Repetition, StaveTempo, StaveText, StaveTie, Volta, Stem, StringNumber, Stroke, System, Tables, TabNote, TabSlide, TabStave, TabTie, TextBracket, TextDynamics, TextFormatter, TextNote, TickContext, TimeSignature, TimeSigNote, Tremolo, Tuning, Tuplet, Vibrato, VibratoBracket, Voice };
export { Vex, Flow };
export default Vex;
In 5.0, I'd prefer to make the ES module the default, so we can remove this hack. Then we can have webpack do a separate build for the CJS files.
@0xfe you commented on the beta merge about the ESM hack. I'm 90% of the way to a clean solution already. I didn't want to delay the beta launch though so I left the hack in.
My proposal for clean ESM support (with compatibility for legacy CJS):
The vexflow/build/ folder will contain:
docs/ <== generated by typedoc
esm/ <== generated by tsc for now. maybe will move to webpack later if we need more customization.
cjs/ <== bundled by webpack
Inside the cjs/ folder are the webpack generated bundles compatible with the ol' reliable <script>
tags.
vexflow-core-with-bravura.js
vexflow-core-with-bravura.js.map
vexflow-core-with-gonville.js
vexflow-core-with-gonville.js.map
vexflow-core-with-petaluma.js
vexflow-core-with-petaluma.js.map
vexflow-core.js
vexflow-core.js.map
vexflow-debug-with-tests.js
vexflow-debug.js
vexflow-font-bravura.js
vexflow-font-bravura.js.map
vexflow-font-custom.js
vexflow-font-custom.js.map
vexflow-font-gonville.js
vexflow-font-gonville.js.map
vexflow-font-petaluma.js
vexflow-font-petaluma.js.map
vexflow.js
vexflow.js.map
Inside the esm/ folder are individual .js files and .d.ts files generated by tsc.
index.js
vex.js
flow.js
stave.js
.... etc ...
Depending on my tests, the *.d.ts files will sit side by side with the ESM js files, or maybe they'll live in their own build/types/ folder.
I will submit a PR for this in the next couple of days after it's tested more, so it can be a part of 4.0 beta 2.
The script tag files will be served from unpkg as follows:
https://unpkg.com/vexflow@4.0.0/build/cjs/vexflow.js
But if we npm install vexflow and import Vex from "vexflow"
we will automatically import the individual files from the vexflow/build/esm/* folder. This will allow the developer's chosen bundler (esbuild / webpack / rollup / etc) to do dead code elimination if possible, for smaller bundles.
One current roadblock is that when compiling TypeScript down to ES module files, we need to add *.js extensions to all the imports.
This issue is known and Microsoft says they aren't planning to add a compiler option for it. https://github.com/microsoft/TypeScript/issues/16577
TypeScript:
import { Fraction } from './fraction';
Output JS file (ESM):
import { Fraction } from './fraction';
The above fails. It needs to look like:
import { Fraction } from './fraction.js';
If anyone knows a good library that will do the rewrites for us, let me know. :-) I've found a couple but none have looked that promising. We might have to write our own imports rewriter to append .js to all the imports in ESM files.
To be frank, the above would be a dealbreaker for me (if i would consider migrating my project to ESModule). The whole ESModule thing seems unfinished on their side (tools/typescript), to be honest, I wouldn't like to use it in production yet. I'll probably try to use Vexflow 4 with CommonJS first. ESModule also needs adaptation in various places in OSMD, and prevents users from using it in a CommonJS way, e.g. in simple node scripts, via require or script tag. But that's just my point of view. I'm sure we'll also transition to ESModule at some point and that it has many advantages, but currently it looks undercooked.
@sschmidTU In the migration OSMD, the usage of ESModule rather resolved issues. The latest migration PR that I created is much simpler that the previous ones. What are the problems? Perhaps we can help. Seems to be, as you say, a complex issue though. https://techsparx.com/nodejs/esnext/esm-to-cjs.html
I'll probably try to use Vexflow 4 with CommonJS first.
Yeah this is totally fine and we're not removing CJS any time soon. (It's how VexFlow has always been deployed after all.)
The TS => ESM conversion has two solutions:
1) Rewrite our TypeScript source files to always include a .js extension. Apparently this is legal TS, but we'd have to make sure our webpack resolves the imports correctly. So if we do import { Accidental } from './accidental.js'
and the JS file doesn't exist, it will load the TS file.
2) Do what we are doing today, but rewrite the output JS files to add the *.js extensions.
I've already written a script to do Solution 2, and it works pretty well in my tests (so far). Hopefully once I'm done with this PR you can give the ESM imports a try. Your webpack bundler can still bundle the VexFlow ESM classes into a OSMD CJS library in the end though, so your users shouldn't be impacted.
Thanks Ron! 1 is quite interesting, this is worth a try, there are not many imports in OSMD.
How would you translate? I use now:
import * as VF from "vexflow";
Solution 1 means that in VexFlow, we always end our imports with a .js. I figured that would be kind of annoying, especially since VSCode doesn't do it automatically when using auto complete.
Anyways, we can discuss further once I post the next beta for you all to test, with the updated ESM support. I think the rewrite imports/exports solution works OK for now.
My two solutions aren't applicable to OSMD or any code that imports VexFlow. This is purely talking about how we can export VexFlow as ESM. The import statements inside ESM files need to have .js extensions. Solution 1 and 2 above are just different ways to add those .js extensions into the import statements.
Inside accidental.ts, we have
import { Fraction } from './fraction';
import { Glyph } from './glyph';
import { Modifier } from './modifier';
import { ModifierContextState } from './modifiercontext';
...
In solution 1, we would manually edit our TypeScript files to look like:
import { Fraction } from './fraction.js';
import { Glyph } from './glyph.js';
import { Modifier } from './modifier.js';
import { ModifierContextState } from './modifiercontext.js';
...
We would have to tell WebPack to look for ts files instead of js files, even though we specify js extensions.
Then when we use tsc to build the ES modules, the imports still retain the .js extensions, so they will work fine.
Instead of doing that, I opted for solution 2, where I let tsc emit the imports without any extensions.
Then I have a script that goes into the build/esm/
folder and adds those extensions. Yes, it uses regex and has potential to be brittle if we do something unexpected with our imports, but I think it'll work for now. Besides, solution 1 is also brittle for different reasons (e.g., VS code autocomplete will not add .js extensions, so if we forget to do it manually, it'll break the resulting ESM file).
@sschmidTU In the migration OSMD, the usage of ESModule rather resolved issues. The latest migration PR that I created is much simpler that the previous ones. What are the problems? Perhaps we can help.
@rvilarl As I said, you can't use require
on an ESModule, so the generateImages
node script doesn't work as it currently is in OSMD, probably needs to be converted to an ESModule. Interestingly, our plain javascript (RawJavascript) example project works with it, using a script tag. It doesn't even need type=module
, so I guess the browser figures that out on its own. But we can continue the discussion in your OSMD PR.
Hello Simon @sschmidTU,
VexFlow 4 will support both CJS and ESM, so OSMD is fine either way.
In fact it is possible to slowly transition your project over to ES modules while keeping some of your scripts as common JS. It is done via the package.json type field: https://nodejs.org/api/packages.html#type
If you specify "type": "module", all JS files in that folder (or its descendant folders) will be treated as ES modules. If you omit this field from the package.json, or specify "type": "commonjs", all JS files will be treated as Common JS.
But what if your project is ESM, but you have a few scripts you have not yet migrated? Then instead of using the .js file extension, you should use the .cjs file extension. That will tell node to treat it as a common JS file, even if it is inside a ESM project.
On the other hand, if your project is CJS, then .js files are treated as common JS by default. If you happen to have one ES module in that project, you can use the .mjs extension. Then Node will treat that file as a ES module.
It looks like OSMD does not specify the package.json type field, so all JS scripts in your project are assumed to be CJS (from Node's perspective). If you want to slowly migrate over some test scripts, just to try ESM out, you can change the file extension to *.mjs, so you can use import { Vex } from "vexflow" to pull in the ESM files (once we get to beta 2 with better ESM support).
Here's a project I'm a fan of that is ESM by default. In their package.json, they specify "type": "module". That means all *.js files will be ESM files. However, they still have some common JS scripts that use require(), such as their increment version script: https://github.com/Tonejs/Tone.js/blob/dev/scripts/increment_version.cjs
These common JS files must have the *.cjs extension, because they live within a ESM project. Thus, whenever you run node myScript.cjs
, node will allow require() statements.
Summary:
(I'm tagging @rvilarl @0xfe as a FYI).
Ron
@ronyeh Thanks for the info! I did try to rename my script to .cjs
, but it doesn't work. But it's okay, I can just migrate the script to make it work.
The error messages aren't very helpful though:
[OSMD.generateImages] init
(node:19900) UnhandledPromiseRejectionWarning: ReferenceError: self is not defined
at Object.<anonymous> (D:\code\osmd\opensheetmusicdisplay\build\opensheetmusicdisplay.js:10:4)
at Module._compile (internal/modules/cjs/loader.js:1063:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
at Module.load (internal/modules/cjs/loader.js:928:32)
at Function.Module._load (internal/modules/cjs/loader.js:769:14)
at Module.require (internal/modules/cjs/loader.js:952:19)
at require (internal/modules/cjs/helpers.js:88:18)
at init (D:\code\osmd\opensheetmusicdisplay\test\Util\generateImages_browserless.cjs:163:12)
at Object.<anonymous> (D:\code\osmd\opensheetmusicdisplay\test\Util\generateImages_browserless.cjs:401:1) at Module._compile (internal/modules/cjs/loader.js:1063:30)
Here's line 10 (in the non-minified build) where self is indeed not defined beforehand (I'm guessing some webpack/module magic):
The calling line 163:
OSMD = require(`${osmdBuildDir}/opensheetmusicdisplay.js`); // window needs to be available before we can require OSMD
This just seems to be an incompatibility with how OSMD is built after adapting Vexflow 4, so it won't work in cjs scripts. It may be a different issue introduced in the Vexflow 4 PR, but it looks like an ESModule issue.
But again, it's probably more work to fix this than to transition my script to ESModule and use import statements instead of requires.
By the way, I also can't require
versions ^3.0 of cross-blob (an npm module), because that's an ESModule too now, so I guess I'll have to migrate anyways. (otherwise i'd have to pin cross-blob e.g. to ^1.2.0, which still supports require
)
edit: Rodrigo migrated the script to .mjs, it works with imports now :)
@ronyeh - coming late to this: a challenge I'm facing in migration to the 4.0 beta is that the shapes of the CJS and ESM builds are different. The ESM build makes available a named export for each class via src/index.tx
, whereas the CJS build only exports these in the namespace Vex.Flow via src/vex.ts
.
The particular situation I'm in is an SSR app, using Vite which transforms ESM import style code to CJS require style code in node to provide the server-side render. As a result the code below works on the client, but not server:
import { Annotation } from 'vexflow';
// becomes something like `const Annotation = require('vexflow').Annotation` on server, which is undefined since we only export the default Vex object.
I wonder if we might consider adding named exports to src/vex.ts
to match the available exports from src/index.ts
in order to provide a similarly shaped build? (Since CJS won't support tree shaking anyway, and the classes are already referenced on the CJS Flow export, it shouldn't change bundle size.)
I've been improving the build process so that projects old and new can integrate with VexFlow 4.
Different projects want to integrate with VexFlow in different ways.
<script src="vexflow.js"></script>
require('vexflow.js')
import Vex from 'vexflow'
import { Vex, Accidental, Stave, ... } from 'vexflow'
Our current 4.0.0 alpha branch supports all of these cases, but emitting the ESM files (ES modules) required a small hack in the Gruntfile.js.
After 4.0, and maybe for 5.0, we should revisit whether the default build product should instead be ESM files. Currently, our default build product is compatible with require and script tags, but not with named imports (the last bullet point above).
The way to move toward ESM as a default:
type: "module"
to the package.json file. https://nodejs.org/api/packages.html#typeThis is not a super high priority issue, but I think it would be a nice improvement for VexFlow going forward, as more and more libraries move toward native ESM support. (Note: I picked up this idea from studying how some other libraries package their NPM builds, e.g., Tone JS: https://github.com/Tonejs/Tone.js).