🎯 ESM Support for PnP

[zdm]

Edit 2021-04-14 by @arcanis: In case you reach this thread by Twitter, ESM for PnP is on our roadmap. It's tricky due to some primitives currently missing in Node (they're working on this, but as you can see it's still branded as experimental), in the meantime if you need it then the node-modules linker remain a perfectly fine solution. In any case, help is appreciated, Twitter rants less.

---

What package is covered by this investigations?```
@softvisio/cli

Describe the goal of the investigation

I am trying to run script, that uses es6 syntax.

yarn add @softvisio/cli
yarn softvisio-cli

But get this error:

(node:4268) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
d:\downloads\yarn\@softvisio-cli-npm-0.1.0-efe3491d08-1.zip\node_modules\@softvisio\cli\bin\cli.js:3
import softvisioCliApi from "../lib/api.js";
^^^^^^

I am using node 13 that can load es6 modules directly.

Is it currently possible to use pure es6 modules with yarn 2 or I need to compile them to commonjs before?

[arcanis]

ESM modules go through a completely different codepath in Node (cjs / esm). The problem is that this codepath is still very new, and although modules have technically been marked stable the way to alter the resolution (aka loaders) is still in heavy discussion (nodejs/modules#351).

So we can't support them at the moment (too early), but as soon as the loader spec moves forward with an implementation we'll be ready to experiment with them 🧪

[zdm]

Thank you.
Please, don't close this issue until this feature will be implemented.

[bgotink]

I looked into the API a bit using Node 14. I've written a PoC implementation at (bgotink@34c0a7b) along with a small test repo over at https://github.com/bgotink/berry-esm.

The API is still unstable, but I think it's worth checking out to see whether there's any valuable feedback to gain from trying. I'm not sure how volatile we should consider the API, so I'm not considering "should we ship this?" at the moment, only "does it work?"

The interesting bits so far:

• It works.
• Resolving a CommonJS dependency using {format: 'commonjs'} in the getFormat hook gives us no more hooks into the file's resolution, so we can't load it from the yarn cache's zipfiles. The same goes for JSON files and {format: 'json'}. All of these need to go through {format: 'dynamic'} which is kinda sad because that means we need to replicate the Node behaviour w.r.t. named exports in these kinds of files.
• This will go nicely with support for package exports ([Case Study] Exports Support #650), where the regular .pnp.c?js file asks for CJS files and the ESM loader uses import. In fact, I would assume most packages that want to adopt ESM will do this in a backwards compatible way, in which case we need package exports to load the ESM variant of the library.

The boring bits:

• Passing the --experimental-loader flag logs a warning regardless of whether said loader is actually ever used. In other words, so to not harass users not interested in experimental ESM support we should make this opt-in
• This is probably not compatible with the native ESM loader. For instance, the loader powered by PnP is quite lenient w.r.t. file extensions, while the native ESM loader isn't.
• Some things don't work yet, e.g. import(await import.meta.resolve('typescript')) fails at the moment because the ?yarn-cache flag gets lost in the resolver
• Stuff like import(pathToFileURL(createRequire(import.meta.url).resolve('typescript'))) doesn't work.
• I've explicitly kept the pnp-loader static and use the PnP API exposed from the .pnp.c?js file

Improvements to the PnP API that would make the PoC a lot cleaner:

• Ideally we'd store the package's type in the PackageInformation exposed by PnP, instead of having to read it from the manifest file ourselves. (The same will go for package exports.)
• It would be great if pnp could be given an option to make file extensions required when directly referencing a file
• A function to see whether a given file path is part of the yarn cache or not would come in handy, that would allow us to use regular file: URLs instead of using a ?yarn-cache query parameter.

A node module with code below works fine in the berry root folder and showcases a lot of the basic functionality already.

// named exports from fs still work even though we patch fs
import fs, {readFileSync} from 'fs';

// JSON can be imported
import pkg                from 'typescript/package';
// CJS package import
import typescript         from 'typescript';

// importing a non-patched builtin
import {fileURLToPath}    from 'url';

console.log('');
console.log(pkg.name, typescript.version);

// Resolves to a regular file: URL
const typescriptUrl = await import.meta.resolve('typescript');
// So it can get transformed into a path and used in fs APIs or even require (BAD IDEA)
const typescriptPath = fileURLToPath(typescriptUrl);

console.log(`url:`, typescriptUrl);
console.log(`path:`, typescriptPath);

const eslintPackagePath = fileURLToPath(await import.meta.resolve('eslint/package'));

// Just to prove that the fs APIs are patched
console.log('fs.readFileSync', JSON.parse(fs.readFileSync(eslintPackagePath)).version);
console.log('readFileSync', JSON.parse(readFileSync(eslintPackagePath)).version);

$ node --experimental-loader ./.pnp-loader.mjs --experimental-import-meta-resolve --harmony-top-level-await test.mjs
(node:68602) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

typescript 3.8.3
url: file:///Users/bram/Workspaces/Private/berry/.yarn/cache/typescript-patch-c42e3ab680-2.zip/node_modules/typescript/lib/typescript.js?yarn-cache=
path: /Users/bram/Workspaces/Private/berry/.yarn/cache/typescript-patch-c42e3ab680-2.zip/node_modules/typescript/lib/typescript.js
fs.readFileSync 6.8.0
readFileSync 6.8.0

[arcanis]

Nice analysis! You mentioned ?yarn-cache - why is this query parameter required? Regular paths currently seem to work fine 🤔

Ideally we'd store the package's type in the PackageInformation exposed by PnP, instead of having to read it from the manifest file ourselves. (The same will go for package exports.)

That seems acceptable 👍

It would be great if pnp could be given an option to make file extensions required when directly referencing a file

Calling resolveToUnqualified might have the same effect as such a flag. Worth a try.

[bgotink]

You mentioned ?yarn-cache - why is this query parameter required? Regular paths currently seem to work fine 🤔

I've kept the module loader conservative in which paths it actually tries to handle itself versus which paths it lets the default loader handle. This requires the different hooks to be able to check whether a URL points to a path that needs to be loaded via the patched filesystem.
The ?yarn-cache query param was a quick and dirty solution for that. Another possibility is an API to see whether a given path requires loading via our patched fs module. Alternatively we could just load all files via the patched fs, though that makes the loader anything but conservative.
In a first attempt for the loader I used a custom protocol yarn-cache:, but it's a bad idea to stray away from the file: protocol for compatibility reasons: import.meta.resolve(...) and import.meta.url expose the URL of a module, changing that from a file: URL to something else will cause breakage for people using the module's URL in order to e.g. load a data file in the same folder as the code file.

I'm not sure what's the best path forward here: just load everything via our getSource hook or defer to the native hook when possible? (in the latter case we need a mechanism to devise whether a URL should be loaded by us, beit via the current ?yarn-cache query parameter or via an API call)

Calling resolveToUnqualified might have the same effect as such a flag. Worth a try.

resolveToUnqualified doesn't resolve bare imports to their actual files, it resolves typescript to /.../berry/.yarn/cache/typescript-patch-c42e3ab680-2.zip/node_modules/typescript.
At the moment the PoC loader uses resolveQualified to resolve that to an actual file, but that implements too much: it automatically adds file extensions and it resolves folders to files called index.whatever.

As I've stated in my previous comment I've kept the module loader as dumb as possible, it loads everything from the regular .pnp.c?js file. Implementing both the CJS resolution algorithm and the ESM resolution algorithm in the regular pnp file is probably not the best idea. Maybe the solution is to use resolveToUnqualified and then handle the rest of the resolution in the module loader?