feat(pnp): experimental esm support #2161

merceyz · 2020-11-22T22:33:13Z

What's the problem this PR addresses?

PnP doesn't work with ESM

Closes #638

How did you fix it?

Implemented an experimental ESM loader

Checklist

I have read the Contributing Guide.
I have set the packages that need to be released for my changes to be effective.
I will check that all automated PR checks pass before the PR gets reviewed.

packages/plugin-pnp/sources/PnpLinker.ts

packages/plugin-pnp/sources/index.ts

merceyz · 2020-12-21T01:48:53Z

Added the config enableExperimentalESMLoader so the loader can be enabled without having the root package.jsons type set to module

nicolo-ribaudo · 2020-12-21T01:54:32Z

I'm not sure exactly why (I'm trying to read Node.js' internals to understand it), but this breaks Node.js' support for named imports from CommonJS modules.
With this file:

import { rollup } from "rollup";

yarn node file.mjs throws that "rollup" doesn't have an export named rollup, but if I use Node.js without yarn (or if I unplug rollup) it works.

EDIT: I am debugging the resolution process, and it seems like that for CJS modules (specifically, I'm debugging .yarn/berry/cache/rollup-npm-2.26.5-e41f5bde2c-7.zip/node_modules/rollup/dist/rollup.js, this call throws, because it uses the native fs (which cannot read in zip files) and not the PnP'ed one.

EDIT2: This was anticipated by @arcanis more than one year ago 😬 nodejs/modules#351 (comment)

I think pnpapi.resolveRequest will need to have support for "exports" in package.json files before landing this PR. The ESM loader should give it the context.conditions parameter that the resolve hook takes, since it tells how to interpret exports. (EDIT: this is tracked at #650)

merceyz · 2020-12-21T02:08:24Z

Yeah, sadly node doesn't let me pass on the entrypoint to a package and have it resolve the rest using exports :(

merceyz · 2020-12-21T11:10:56Z

Almost got it to work with exports support but i'm now blocked by webpack/enhanced-resolve#270

merceyz · 2020-12-21T17:09:40Z

@nicolo-ribaudo Added exports support, fixed module type detection, and file reading so should be good to go now.

Thanks to @sokra for fixing the bug in enhanced-resolve so fast

nicolo-ribaudo · 2020-12-21T17:58:49Z

Thanks! I got yarn gulp build working in the Babel PR 🙂

I had to unplug @rollup/plugin-babel and rollup-plugin-terser because of the CJS detection (but I think that this cannot be fixed until node exposes a way to make it read the CJS source from zip archives?)

I had to unplug gulp, otherwise I get this error:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/home/nicolo/.yarn/berry/cache/gulp-npm-4.0.2-56726bdf31-7.zip/node_modules/gulp/bin/gulp.js' imported from /home/nicolo/Documenti/dev/babel/babel/
Did you mean to import /home/nicolo/.yarn/berry/cache/gulp-npm-4.0.2-56726bdf31-7.zip/node_modules/gulp/bin/gulp.js?
  at finalizeResolution (internal/modules/esm/resolve.js:276:11)
  at moduleResolve (internal/modules/esm/resolve.js:699:10)
  at defaultResolve (internal/modules/esm/resolve.js:810:11)
  at resolve$1 (file:///home/nicolo/Documenti/dev/babel/babel/experimental-pnp-esm-loader.mjs:7094:16)
  at Loader.resolve (internal/modules/esm/loader.js:86:40)
  at Loader.getModuleJob (internal/modules/esm/loader.js:230:28)
  at Loader.import (internal/modules/esm/loader.js:165:28)
  at internal/modules/run_main.js:46:28
  at Object.loadESM (internal/process/esm_loader.js:68:11)

merceyz · 2020-12-21T18:03:29Z

It should be able to detect the correct type either way 🤨

nicolo-ribaudo · 2020-12-21T18:04:15Z

It should be able to detect the correct type either wait 🤨

The problem is that Node.js reads the file source to analyze the exports (here), and it doesn't work with PnP.

In Node.js this works:

// file.js
exports.foo = 2;

// main.mjs

import { foo } from "./foo.js"

but not when foo.js is in a Zip file.

packages/yarnpkg-pnp/sources/esm-loader/loader.ts

arcanis · 2020-12-21T18:11:06Z

The problem is that Node.js reads the file source to analyze the exports (here), and it doesn't work with PnP.

That's because they destructure fs, thus making it impossible to replace the implementation with a zip-compatible version. We probably should open a discussion about this to make sure this use case is formally acknowledged 😕

merceyz · 2020-12-21T20:46:31Z

Got commonjs to work on babel/babel#12296 so yarn gulp runs (EDIT: but crashes) without unplugging anything 🎉

packages/yarnpkg-pnp/sources/esm-loader/loader.ts

franciscofabian · 2020-12-22T19:20:43Z

This works great for me. Thanks!

bgotink · 2020-12-22T20:01:36Z

What kind of feature parity are we looking to achieve with node's builtin ESM resolution?

By default the builtin esm loader doesn't add any extensions and it doesn't do directory lookup, though this behaviour can be enabled via an experimental flag.

Difference in behaviour

Setup:

cd $(mktemp -d)
echo 'yarnPath: /path/to/yarn.js' > .yarnrc.yml
touch yarn.lock
yarn config set enableExperimentalESMLoader true
yarn
mkdir -p node_modules/test
echo '{"type": "module"}' > node_modules/test/package.json
echo 'export foo = 1;' > node_modules/test/foo.js
cat <<EOF >index.mjs
import {foo} from 'test/foo';
console.log(foo);
EOF

Default behaviour in node:

$ node index.mjs
internal/process/esm_loader.js:74
    internalBinding('errors').triggerUncaughtException(
                              ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/private/var/folders/_d/ch2kc4h960d10cy_2c41qqzw0000gn/T/tmp.UJ1D7XHXnZ/node_modules/test/foo' imported from /private/var/folders/_d/ch2kc4h960d10cy_2c41qqzw0000gn/T/tmp.UJ1D7XHXnZ/index.mjs
Did you mean to import test/foo.js?
    at finalizeResolution (internal/modules/esm/resolve.js:276:11)
    at moduleResolve (internal/modules/esm/resolve.js:699:10)
    at Loader.defaultResolve [as _resolve] (internal/modules/esm/resolve.js:810:11)
    at Loader.resolve (internal/modules/esm/loader.js:86:40)
    at Loader.getModuleJob (internal/modules/esm/loader.js:230:28)
    at ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:56:40)
    at link (internal/modules/esm/module_job.js:55:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}

vs with yarn:

$ yarn node index.mjs
(node:20409) 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)
1

or node's experimental specifier resolution:

$ node --experimental-specifier-resolution=node index.mjs
1

Node by default doesn't support loading JSON files, though again this can be enabled via an experimental flag.

Difference in behaviour

Change the `index.mjs` file in the setup above to

import pkg from 'test/package.json';

console.log(pkg);

and you'll get

 $ node index.mjs
internal/process/esm_loader.js:74
    internalBinding('errors').triggerUncaughtException(
                              ^

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".json" for 
/private/var/folders/_d/ch2kc4h960d10cy_2c41qqzw0000gn/T/tmp.UJ1D7XHXnZ/node_modules/test/package.json
    at Loader.defaultGetFormat [as _getFormat] (internal/modules/esm/get_format.js:71:15)
    at Loader.getFormat (internal/modules/esm/loader.js:102:42)
    at Loader.getModuleJob (internal/modules/esm/loader.js:231:31)
    at async ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:58:21)
    at async Promise.all (index 0)
    at async link (internal/modules/esm/module_job.js:63:9) {
  code: 'ERR_UNKNOWN_FILE_EXTENSION'
}

vs

$ yarn node index.mjs
(node:20808) 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)
{ type: 'module' }

and

$ node --experimental-json-modules index.mjs
(node:20904) ExperimentalWarning: Importing JSON modules is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
{ type: 'module' }

IMO it's dangerous to deviate from node's standard in these cases. That would quickly lead to "it works in PnP but not via node-modules/npm/pnpm or vice versa" kinds of problems.
Even if we assume node will adopt these behaviours by default at some point in the future, I would still be cautious about enabling them by default already.

merceyz · 2020-12-22T23:01:01Z

What kind of feature parity are we looking to achieve with node's builtin ESM resolution?

I'd like to have it at a level where what works without PnP works with it, nothing more

By default the builtin esm loader doesn't add any extensions and it doesn't do directory lookup

What a twist, PnP was more forgiving for once 😄, fixed.

Node by default doesn't support loading JSON files, though again this can be enabled via an experimental flag.

Fixed

packages/yarnpkg-pnp/sources/esm-loader/loader.ts

packages/plugin-pnp/sources/PnpLinker.ts

packages/plugin-pnp/sources/index.ts

packages/yarnpkg-pnp/sources/esm-loader/loader.ts

arcanis · 2021-10-13T15:23:43Z

packages/yarnpkg-pnp/sources/loader/applyPatch.ts

@@ -391,5 +394,19 @@ export function applyPatch(pnpapi: PnpApi, opts: ApplyPatchOptions) {
    return false;
  };

+  // Specifying the `--experimental-loader` flag makes Node enter ESM mode so we change it to not do that


Can you add inside the comment a reference to the test that would break if this part of the code was removed?

Sure, added the upstream issue as well.
nodejs/node#33226

I've added integration tests that cover the same things

arcanis

Looks good; I'm a little worried about the auto-detect, but it's not clear whether it'll cause issues in practice.

arcanis · 2021-10-20T11:46:39Z

packages/acceptance-tests/pkg-tests-specs/sources/pnp-esm.test.ts

+        pnpEnableExperimentalEsm: true,
+      },
+      async ({path, run, source}) => {
+        await xfs.writeFilePromise(ppath.join(path, `index` as Filename), `console.log(typeof require === 'undefined')`);


Shouldn't it be index.js (with the extension?)

No, it's testing nodejs/node#33226 where Node incorrectly loads a commonjs file without an extension as an ESM file.
Added a link to the issue, same as #2161 (comment)

arcanis · 2021-10-20T11:48:05Z

packages/acceptance-tests/pkg-tests-specs/sources/pnp-esm.test.ts

+  test(
+    `it should be able to import a dependency`,
+    makeTemporaryEnv(
+      {
+        type: `module`,
+        dependencies: {
+          "no-deps": `1.0.0`,
+        },
+      },
+      async ({path, run, source}) => {
+        await expect(run(`install`)).resolves.toMatchObject({code: 0});
+
+        await xfs.writeFilePromise(
+          ppath.join(path, `index.js` as Filename),
+          `import noDeps from 'no-deps/index.js';\nconsole.log(noDeps)`,
+        );
+
+        await expect(run(`node`, `./index.js`)).resolves.toMatchObject({
+          code: 0,
+          stdout: `{ name: 'no-deps', version: '1.0.0' }\n`,
+        });
+      },
+    ),
+  );


Would be worth adding a test to make sure that ESM packages can import other ESM packages

acusti · 2021-10-20T14:19:59Z

niiiiiiiiiiiiice. congrats @merceyz and team 🥳! your effort is much appreciated.

nicolo-ribaudo · 2021-10-20T14:25:39Z

Awesome! 🎉

merceyz mentioned this pull request Nov 22, 2020

🎯 ESM Support for PnP #638

Closed

bgotink reviewed Nov 23, 2020

View reviewed changes

packages/plugin-pnp/sources/PnpLinker.ts Outdated Show resolved Hide resolved

merceyz force-pushed the merceyz/esm-loader branch 2 times, most recently from b043a03 to cf42a1d Compare November 29, 2020 13:38

merceyz force-pushed the merceyz/esm-loader branch from cf42a1d to aa834f8 Compare December 5, 2020 00:05

merceyz force-pushed the merceyz/esm-loader branch from aa834f8 to 7bc1960 Compare December 13, 2020 15:58

nicolo-ribaudo reviewed Dec 20, 2020

View reviewed changes

packages/plugin-pnp/sources/index.ts Outdated Show resolved Hide resolved

nicolo-ribaudo mentioned this pull request Dec 20, 2020

Use native ESM for dev scripts babel/babel#12296

Merged

merceyz force-pushed the merceyz/esm-loader branch 2 times, most recently from 1b86f68 to 3201a51 Compare December 21, 2020 01:28

nicolo-ribaudo reviewed Dec 21, 2020

View reviewed changes

packages/yarnpkg-pnp/sources/esm-loader/loader.ts Outdated Show resolved Hide resolved

merceyz force-pushed the merceyz/esm-loader branch from 054f61b to d523789 Compare December 21, 2020 20:48

nicolo-ribaudo reviewed Dec 21, 2020

View reviewed changes

packages/yarnpkg-pnp/sources/esm-loader/loader.ts Outdated Show resolved Hide resolved

merceyz force-pushed the merceyz/esm-loader branch from a7110bc to ce798fb Compare December 22, 2020 00:08

nicolo-ribaudo reviewed Dec 22, 2020

View reviewed changes

packages/yarnpkg-pnp/sources/esm-loader/loader.ts Outdated Show resolved Hide resolved

merceyz commented Dec 22, 2020

View reviewed changes

packages/yarnpkg-pnp/sources/esm-loader/loader.ts Outdated Show resolved Hide resolved

arcanis reviewed Dec 25, 2020

View reviewed changes

packages/plugin-pnp/sources/PnpLinker.ts Outdated Show resolved Hide resolved

packages/plugin-pnp/sources/index.ts Outdated Show resolved Hide resolved

packages/yarnpkg-pnp/sources/esm-loader/loader.ts Outdated Show resolved Hide resolved

merceyz added 2 commits October 12, 2021 15:43

chore: update versions

62d3e18

Merge branch 'master' into merceyz/esm-loader

4a87fe3

arcanis reviewed Oct 13, 2021

View reviewed changes

merceyz added 8 commits October 13, 2021 17:51

chore: link to the test and upstream issue

66c3bd2

Merge branch 'master' into merceyz/esm-loader

8d0eb39

refactor: pass conditions directly to resolve.exports

ae63ff0

ci: set config as env variables

c9e411b

chore: update warning message

f4c801d

refactor: use changeFilePromise to update the loader

f24fab3

ci: remove Astro e2e test

d440279

I've added integration tests that cover the same things

Merge branch 'master' into merceyz/esm-loader

872a974

merceyz marked this pull request as ready for review October 20, 2021 11:12

arcanis previously approved these changes Oct 20, 2021

View reviewed changes

test: add link to issue

61d4a21

merceyz dismissed arcanis’s stale review via 61d4a21 October 20, 2021 12:03

arcanis merged commit 8ca7aef into master Oct 20, 2021

arcanis deleted the merceyz/esm-loader branch October 20, 2021 14:07

merceyz mentioned this pull request Oct 20, 2021

feat(pnp): support consolidated ESM loader hooks #3603

Merged

3 tasks

adalinesimonian mentioned this pull request Oct 22, 2021

Support resolving Stylelint using PnP stylelint/vscode-stylelint#272

Closed

merceyz added the esm label Oct 28, 2021

natemoo-re mentioned this pull request Oct 29, 2021

🐛 BUG: Astro packages don't properly define their dependencies (Yarn PnP incompatibilty) withastro/astro#1676

Closed

alan-agius4 mentioned this pull request Dec 16, 2021

Yarn berry PnP - ng serve fails for new project angular/angular-cli#22386

Closed

15 tasks

merceyz mentioned this pull request Jan 6, 2022

fix(plugin-nm): update data key version #3950

Merged

3 tasks

merceyz mentioned this pull request Jan 22, 2022

[Feature] Support custom export resolution strategy #2815

Closed

2 tasks

avarun42 mentioned this pull request Mar 4, 2022

Add Yarn 2 section to FAQ sveltejs/kit#1027

Merged

5 tasks

mxro mentioned this pull request Nov 3, 2022

SvelteKit Support goldstack/goldstack#266

Open

merceyz mentioned this pull request Jul 21, 2023

feat(pnp): enable ESM support by default #5574

Open

3 tasks

maximdeclercq mentioned this pull request Jan 10, 2024

chore: use peer dependencies rather than dev dependencies sveltejs/kit#11433

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pnp): experimental esm support #2161

feat(pnp): experimental esm support #2161

merceyz commented Nov 22, 2020 •

edited

merceyz commented Dec 21, 2020

nicolo-ribaudo commented Dec 21, 2020 •

edited

merceyz commented Dec 21, 2020

merceyz commented Dec 21, 2020

merceyz commented Dec 21, 2020 •

edited

nicolo-ribaudo commented Dec 21, 2020 •

edited

merceyz commented Dec 21, 2020 •

edited

nicolo-ribaudo commented Dec 21, 2020 •

edited

arcanis commented Dec 21, 2020

merceyz commented Dec 21, 2020 •

edited

franciscofabian commented Dec 22, 2020

bgotink commented Dec 22, 2020

merceyz commented Dec 22, 2020

arcanis Oct 13, 2021

merceyz Oct 13, 2021

arcanis left a comment

arcanis Oct 20, 2021

merceyz Oct 20, 2021

arcanis Oct 20, 2021

acusti commented Oct 20, 2021

nicolo-ribaudo commented Oct 20, 2021

feat(pnp): experimental esm support #2161

feat(pnp): experimental esm support #2161

Conversation

merceyz commented Nov 22, 2020 • edited

merceyz commented Dec 21, 2020

nicolo-ribaudo commented Dec 21, 2020 • edited

merceyz commented Dec 21, 2020

merceyz commented Dec 21, 2020

merceyz commented Dec 21, 2020 • edited

nicolo-ribaudo commented Dec 21, 2020 • edited

merceyz commented Dec 21, 2020 • edited

nicolo-ribaudo commented Dec 21, 2020 • edited

arcanis commented Dec 21, 2020

merceyz commented Dec 21, 2020 • edited

franciscofabian commented Dec 22, 2020

bgotink commented Dec 22, 2020

merceyz commented Dec 22, 2020

arcanis Oct 13, 2021

Choose a reason for hiding this comment

merceyz Oct 13, 2021

Choose a reason for hiding this comment

arcanis left a comment

Choose a reason for hiding this comment

arcanis Oct 20, 2021

Choose a reason for hiding this comment

merceyz Oct 20, 2021

Choose a reason for hiding this comment

arcanis Oct 20, 2021

Choose a reason for hiding this comment

acusti commented Oct 20, 2021

nicolo-ribaudo commented Oct 20, 2021

merceyz commented Nov 22, 2020 •

edited

nicolo-ribaudo commented Dec 21, 2020 •

edited

merceyz commented Dec 21, 2020 •

edited

nicolo-ribaudo commented Dec 21, 2020 •

edited

merceyz commented Dec 21, 2020 •

edited

nicolo-ribaudo commented Dec 21, 2020 •

edited

merceyz commented Dec 21, 2020 •

edited