feat: support yarn PnP out of box, propagate PnP runtime

[noahnu]

If using https://yarnpkg.com/advanced/pnpapi, we need to propagate it to the worker. This adds Yarn PnP support to synckit.

Another solution is to just propagate the NODE_OPTIONS env var from the parent (this is probably a better solution).

[changeset-bot]

🦋 Changeset detected

Latest commit: 1b43e6d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
synckit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[JounQin]

How about this one?

[github-actions]

📊 Package size report   No changes

File	Before	After
Total (Includes all files)	39.7 kB	39.7 kB
Tarball size	11.1 kB	11.1 kB

Unchanged files

File	Size
lib/index.cjs	8.2 kB
lib/index.d.ts	1.2 kB
lib/index.js	8.9 kB
lib/index.js.map	7.6 kB
lib/types.d.ts	936 B
lib/types.js	44 B
lib/types.js.map	102 B
LICENSE	1.1 kB
package.json	4.0 kB
README.md	7.6 kB

_{🤖 This report was automatically generated by pkg-size-action}

[JounQin]

Another solution is to just propagate the NODE_OPTIONS env var from the parent (this is probably a better solution).

NODE_OPTIONS seems much difficult to maintain because there could be things nothing related. Why do you think it could be the better solution?

[codecov]

Codecov Report

Merging #98 (f0ed716) into main (59af559) will decrease coverage by 4.68%.
The diff coverage is 14.28%.

❗ Current head f0ed716 differs from pull request most recent head 1b43e6d. Consider uploading reports for the commit 1b43e6d to get more accurate results

@@             Coverage Diff             @@
##              main      #98      +/-   ##
===========================================
- Coverage   100.00%   95.31%   -4.69%     
===========================================
  Files            2        2              
  Lines          121      128       +7     
  Branches        55       59       +4     
===========================================
+ Hits           121      122       +1     
- Misses           0        5       +5     
- Partials         0        1       +1     

Impacted Files	Coverage Δ
src/index.ts	95.27% <14.28%> (-4.73%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[noahnu]

I'll take a look tomorrow to see about adding a test case. There might be a better way to setup the runtime as well. I can check with the yarn maintainers.

[JounQin]

Besides, maybe you can take privatenumber/get-tsconfig#28 and privatenumber/get-tsconfig#32 as reference.

[lachlanhunt]

@JounQin this would almost fix the issue discussed in microsoft/vscode-eslint#1494. but I think because worker.mjs in the eslint-import-resolver-typescript is using ESM, it would also need the --experimental-loader flag added to execArgv with the path to .pnp.loader.mjs, if present, for it to fully solve the problem. I tried patching this locally with that extra flag and it worked.

[JounQin]

@lachlanhunt If you want to progress it faster, you can raise a new PR to continue @noahnu's work.

[noahnu]

Latest changes I pushed up seem to work even without the experimental loader added. I've posted in the yarn discord to get some feedback. Looking at test coverage now.

NODE_OPTIONS seems much difficult to maintain because there could be things nothing related. Why do you think it could be the better solution?

Devs use NODE_OPTIONS to configure the node process. When using child_process, these options propagate to child processes by default. I suspect folks would expect the same to be true of any workers spun out from a parent process.

[noahnu]

hmm, not sure the best way to test this

[lachlanhunt]

hmm, not sure the best way to test this

Here's a simple test script I wrote while I was investigating the issue I encountered with the import resolver. It's based on the ESLint SDK script that yarn generates, and is able to show the same error that the ESLint extension shows in VSCode. You may be able to adapt this to run in the test suite, and to use synckit directly (rather than via the import resolver).

const { existsSync } = require(`fs`);
const { createRequire } = require(`module`);
const { resolve } = require(`path`);

const relPnpApiPath = '.pnp.cjs'; // Set this relative path to wherever .pnp.cjs is located.

const absPnpApiPath = resolve(__dirname, relPnpApiPath);
const absRequire = createRequire(absPnpApiPath);

if (existsSync(absPnpApiPath)) {
  if (!process.versions.pnp) {
    // Setup the environment to be able to require eslint
    require(absPnpApiPath).setup();
  }
}

void absRequire(`eslint-import-resolver-typescript`);

process.on('uncaughtException', err => {
  console.error('Uncaught error from worker', err);
  process.exit(1);
});

// Simple timeout to keep node alive while waiting for the worker thread to throw an error
setTimeout(() => console.log('PASS'), 100);

If you run node path-to-script.js (within a project using PnP), then it outputs the error. If you run yarn node path-to-script.js, then NODE_OPTIONS are set and automatically propagated to the worker, and it outputs PASS. In this case, .pnp.loader.mjs must also be present for it to pass. It fails without it.

[noahnu]

My failed attempt at a test: https://github.com/noahnu/synckit/pull/1/files. I tried just running the sync directly without creating a new process but didn't have any luck so tried just recreating the full environment.. Didn't really work either.

[JounQin]

https://github.com/un-ts/synckit/releases/tag/v0.8.2