-
-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support yarn PnP out of box, propagate PnP runtime #98
Conversation
🦋 Changeset detectedLatest commit: 1b43e6d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this one?
📊 Package size report No changes
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
|
Codecov Report
@@ 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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. |
Besides, maybe you can take privatenumber/get-tsconfig#28 and privatenumber/get-tsconfig#32 as reference. |
@JounQin this would almost fix the issue discussed in microsoft/vscode-eslint#1494. but I think because worker.mjs in the |
@lachlanhunt If you want to progress it faster, you can raise a new PR to continue @noahnu's work. |
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.
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. |
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 |
Co-authored-by: JounQin <admin@1stg.me>
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. |
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).