Add ES Module Support

[brendean]

Adding ES module support to resolve #1093

[sidorares]

Thanks, looks good, let me test locally first and happy to merge then.

[sidorares]

actually, can you think of unit test for this? Should run for node 13+ only

[sidorares]

merging for now, but would be good to add unit test somehow later

[jaydenseric]

Note that package exports are also a Node.js v12 thing, and affect both require and import.

This change needs more work; as it is currently it will stop most imports from working. When you specify an exports field, it means any files not made available via the rules will be inaccessible via require or import.

For example, with:

{
  "exports": {
    "./promise": "./promise.js"
  }
}

Here are some examples that will error:

require('mysql2')
require('mysql2/package.json')
require('mysql2/lib')
require('mysql2/lib/index')
require('mysql2/lib/index.js')
require('mysql2/promise.js')

Shooting from the hip, if you are happy to allow everything in lib to be public, you could try:

{
  "exports": {
    ".": "./lib/index.js",
    "./lib/": "./lib/",
    "./promise": "./promise.js"
    "./promise.js": "./promise.js"
    "./package": "./package.json",
    "./package.json": "./package.json"
  }
}

You could choose not to do the additional "./promise.js", if you are happy to force people to use an extensionless specifier (and vice versa for "./package") - but that would be a breaking change that should be documented.

[sidorares]

thanks for heads up @jaydenseric! Only 2 entry points are documented to work, mysql2 and msql2/promise. Never tested this module with esm myself, do you see any issues when used only as mysql2 / mysql2/promise ?

[jaydenseric]

Only 2 entry points are documented to work

Anything declared in package.json files and main is the public API, and explicitly documented or not in the real world people count on being able to use anything published to npm:

https://unpkg.com/browse/[email protected]/

When moving to package exports, publish a semver major and declare the breaking changes.

do you see any issues when used only as mysql2 / mysql2/promise ?

I don't think require('mysql2') will work without adding ".": "./lib/index.js".

Note that ESM can only import from CJS files with a default import, so to support more ergonomic ESM named imports you need to use conditional package exports, and have seperate .js/.cjs and .mjs files. Here is how I do it:

https://github.com/jaydenseric/extract-files/blob/v8.1.0/package.json#L33

Note that conditional package exports breaks Node.js v13-13.6:

https://github.com/jaydenseric/extract-files/blob/v8.1.0/package.json#L43

There is much to discuss on this topic; it's enormously complex. Unfortunately almost everything you see or read in the wild is now outdated/wrong.

[sidorares]

Do you have any experience unit testing all this complexity @jaydenseric ? Don't want to test this manually more than once

[jaydenseric]

Adequately explaining the testing also involves explaining the best practices of how it all works, which I don't have the time to do here, sorry. I don't actually use this package; I just noticed this PR get merged and thought to give you a heads-up about the gotchas.

IMO the gold standard is testing the real ESM functionality of Node.js without any transpilation or money-patching. This particularly helps detect invalid importing of dependencies that would be tolerated by Babel/Webpack. To do that without abandoning old Node.js or CJS, a few things need to happen.

First, here is a package test scripts excerpt:

https://github.com/jaydenseric/extract-files/blob/v8.1.0/package.json#L77

{
  "test:esm": "if node --input-type=module -e '' > /dev/null 2>&1; then coverage-node -r hard-rejection/register test/index.mjs; fi",
  "test:cjs": "coverage-node -r hard-rejection/register test",
}

If Node.js supports ESM natively the test:esm script will do it's thing, otherwise it will be skipped.

If you publish any ESM .mjs files, it's good to test them which requires writing tests in propper ESM .mjs files too, as CJS can't require ESM. To do this, I write my tests in ESM and transpile them to also be available in CJS, so both can be tested separately:

https://github.com/jaydenseric/extract-files/tree/v8.1.0/src/test

Note that for lib code, with the exception of entrypoint files you want named exports to work from, you don't want sibling .mjs/.js files or you will end up hitting the dual package hazard:

https://nodejs.org/api/esm.html#esm_dual_package_hazard

I buy into strategy 1:

https://nodejs.org/api/esm.html#esm_approach_1_use_an_es_module_wrapper

This has implications for tree-shaking, but IMO this is balanced out by removing the chance that separate require and import of the same API via deep imports won't result in bundle duplication.

Note that package exports only kick in when the package is being required or imported via another package. Although dynamic import works in CJS, you can't just unit test a require and import of each part of the API in a CJS test. I have long wanted to publish a testing tool that does a simulate an npm install in a temp dir you can assert import and require against, as that would really smoke out any mistakes with package.json files and exports fields, and also .npmignore. These things are rarely tested and relate to a lot of package breakages.

[sidorares]

I have long wanted to publish a testing tool that does a simulate an npm install in a temp dir you can assert import and require against

that would be super helpful

[mdierolf]

Hi, this PR appears to be causing a regression - import 'mysql2' is not working

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: No "exports" main resolved in node_modules/mysql2/package.json
    at applyExports (internal/modules/cjs/loader.js:487:9)
    at resolveExports (internal/modules/cjs/loader.js:503:23)
    at Function.Module._findPath (internal/modules/cjs/loader.js:631:31)
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:949:27)
    at Function.Module._load (internal/modules/cjs/loader.js:838:27)
    at Module.require (internal/modules/cjs/loader.js:1022:19)
    at require (internal/modules/cjs/helpers.js:72:18)
 {
  code: 'ERR_PACKAGE_PATH_NOT_EXPORTED'
}

Please see PR above for a fix for the issue

[chrisveness]

Just a heads-up: v2.2.0 was a breaking change for me, as I was previously using

import mysql from 'mysql2/promise.js';

which worked fine up to v2.1.0.

From v2.2.0 it is necessary to use

import mysql from 'mysql2/promise';

Perhaps it would be worth adding

"./promise.js": "./promise.js"

to the exports, to maintain compatibility.

[sidorares]

Perhaps it would be worth adding

sounds like an easy fix, can you create PR?