Migrate package to ES6 Module

[yne]

Motivation and Context

Opentype.js codebase is structured as modules (see: import/export lines), but it is not declared as such (type:module in package.json) because we rely on commonJs require() syntax to import node-specific function (ex: fs). This "hybrid" usage forced us to use reify in out tests order to make mocha<=8 work (but mocha 8 also has at least 2 known vulnerabilities)

Sadly this combo don't work anymore with latest mocha v10, so we are stuck on a vulnerable version.

So here are 4 possibles structure we could use:

package type	mocha v10	extensions	require()	Notes
commonjs	:red_circle:	.js	:red_circle:	current state of opentype.js
commonjs	:red_circle:	js=>.mjs	:green_circle:
module	:green_circle:	.js	:red_circle:	can't require(*.js) : .js are now considered ESM
module	:green_circle:	.cjs+.js	:green_circle:	:point_right: solution choosen :point_left:

I used the less breaking options (n°4) :

• browser can still <script src="//cdn.example/opentypejs/dist/opentype.js"></script>
• browser can still import * as opentype from "//cdn.example/opentypejs/dist/opentype.module.js"
• old node can still require('opentype.js') which load the .cjs version (disambiguation required)
• new node can still import * as opentype from 'opentype.js' which load the .module.js version

But declaring as type:module mean that require() must be migrated to import / import().

How Has This Been Tested?

by releasing my 2.0.1 fork on NPM and using it

with a node script main.js

opentype = require('opentype.yne.js');
// import not possible here: "Cannot use import statement outside a module"

with a node script main.mjs

import * as opentype from 'opentype.yne.js';
// require() not possible here: "require is not defined in ES module scope"

with an index.html testing both cjs/jsm cases

<script src="opentype.yne.js/dist/opentype.js"></script>
<script>console.log(opentype);</script>

<script type=module>
import * as ot from "./opentype.yne.js/dist/opentype.module.js";
console.log(ot);
</script>

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [x] Breaking change: removal of .load() / .download() functions

Checklist:

• [x] I did npm run test and all tests passed green (including code styling checks).
• [ ] I have added tests to cover my changes.
• [x] My change requires a change to the documentation.
• [x] I have updated the README accordingly.

[yne]

@ILOVEPIE / @Connum

No urgency (since 2.0.0 is not so near), but I would like to get your feeling about it.

[Connum]

I'm not a fan of dropping those functions. If we really have to, let's at least go for your suggestion:

A less breaking change would be to print a warning in the .load() .download() function to direct the user to the new usage

What about a different approach: Use dynamic imports and declare fs as an external: https://github.com/Connum/opentype.js/tree/esm-dynamic-imports

[yne]

Use dynamic imports

Seems like the best solution so far, but unlike synchronous require(), the import() is asynchronous. This create funny prototype like this

may as well drop the loadSync API for 2.0.0 and only keep .load() / .download() ?

[Connum]

Yeah, I found that funny too. :) but the function still works kind of synchronously itself using await, it just had to be declared as async to be able to use the await keyword.

[Connum]

I'll also take a look at

import { createRequire } from 'module';

Tomorrow, not sure about the node version requirements and browser compatibility yet.

[yne]

it just had to be declared as async to be able to use the await keyword.

That's what async function are for. Having an async loadSync() declaration is laughable.

I tested import { createRequire } from 'module'; and it does not work in browser

If we HAVE to keep those user-side function, can we at least drop the .loadSync() and only keep the promise part of .load() ?

But I'd like to remind that all those functions are a mess to maintain, they rely on vendors API and I was hoping to drop them all for 2.x

[yne]

@Connum Here is what I did:

• Used your import() strategy => work well :+1:, I added an engines field to the new package.json in order to reflect the ES2020 minimum requirement.
• Updated PR description to reflect that .load()/.download() are kept
• Removed the loadSync method since it is not sync anyway (see .load() for the await version)
• add a @deprecated to the .load() and .download() methods so we can show a replacement code.
• use createRequire for importing CJS into ESM test
• move the "no-foreach" rules into no-restricted-syntax declaration since
  eslint-plugin-local-rules (which shall have been declared among devDependencies) don't work with ESM
• Remove the *.js import file extension check because ESM already enforce that.

Tell me if you have any improvement ideas

[luxaritas]

Heads up: Attempting to use this library within my ESM app run through vite-node, and it encounters a runtime error due to the lack of "type": "module" in the package.json. For now I'm using patch-package to manually add it

[ILOVEPIE]

Heads up: Attempting to use this library within my ESM app run through vite-node, and it encounters a runtime error due to the lack of "type": "module" in the package.json. For now I'm using patch-package to manually add it

Heads up: switching to ESM entirely will completely break support for a ton of libraries already built on opentype.js, including mine.

[yne]

@ILOVEPIE are you sure about that breakage ? because in this PR, I took the effort to make it backward compatible. so all previous path are still there and there is now a new opentype.module.js artefact IIRC

[luxaritas]

Definitely not suggesting to remove CJS support, just to get ESM working in conjunction!

[yne]

Shall I rebase/resolve this PR or can it still wait ?

[Connum]

I think we should wait until some of the larger PRs are merged. It's easier to then make the required code changes here than getting all the different PR authors do so.

[yne]

Understood, I'll be ready :)

[ILOVEPIE]

This has been resolved.