fix: fix PnP ESM loader integration#255
Conversation
🦋 Changeset detectedLatest commit: 3057f9c 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 |
WalkthroughThe PR removes conditional Changes
Sequence Diagram(s)sequenceDiagram
participant Worker as Worker Process
participant Helper as setupTsRunner
participant execArgv as execArgv
Note over Worker,Helper: Before: Conditional PnP injection
Worker->>Helper: Check MODULE_REGISTER_SUPPORTED
alt MODULE_REGISTER_SUPPORTED true
Helper->>execArgv: Add PnP loader to execArgv
else MODULE_REGISTER_SUPPORTED false
Helper->>execArgv: Skip PnP loader
end
Note over Worker,Helper: After: Unconditional PnP injection
Worker->>Helper: Initialize
Helper->>execArgv: Always add PnP loader if present
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
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.
Important
Looks good to me! 👍
Reviewed everything up to 3057f9c in 1 minute and 2 seconds. Click for details.
- Reviewed
65lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/honest-forks-juggle.md:1
- Draft comment:
Changelog entry is clear and concise. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/helpers.ts:312
- Draft comment:
Unconditionally prepending the --loader flag ensures the PnP loader is loaded early, resolving the chicken‐and‐egg issue (#254). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining why a change was made. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue.
3. src/index.ts:107
- Draft comment:
Removing the module.register call in the worker thread avoids late loader registration, ensuring modules (like synckit) are resolvable under PnP as intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining the effect of removing a specific call. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue. It seems to violate the rule against purely informative comments.
Workflow ID: wflow_FB8wS5GLUyaUkBzB
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/helpers.ts (1)
601-601: Consider removing unusedpnpLoaderPathfrom workerData.The
pnpLoaderPathis no longer extracted or used in the worker (see Line 108 insrc/index.ts), as the PnP loader is now unconditionally passed viaexecArgv. Passing unused data toworkerDatais unnecessary.Apply this diff to remove the unused field:
- workerData: { sharedBufferView, workerPort, pnpLoaderPath }, + workerData: { sharedBufferView, workerPort },Note: If you make this change, also consider updating the
WorkerDatainterface insrc/types.tsto remove thepnpLoaderPathfield for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/honest-forks-juggle.md(1 hunks)src/helpers.ts(1 hunks)src/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/index.ts (1)
src/types.ts (1)
WorkerData(24-28)
src/helpers.ts (1)
src/constants.ts (1)
LOADER_FLAG(104-104)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
.changeset/honest-forks-juggle.md (1)
1-5: Changeset structure looks good.The changeset is properly formatted with the correct package name, appropriate patch bump type for a bug fix, and a clear description that aligns with the PR objectives to fix PnP ESM loader integration.
src/helpers.ts (1)
315-315: LGTM! Fixes the PnP ESM loader timing issue.Unconditionally adding the PnP loader to
execArgvensures the loader is active before the worker attempts to resolve any dependencies, eliminating the chicken-and-egg problem wheremodule.registerwas called too late.src/index.ts (1)
108-108: LGTM! Correctly removes unused PnP loader path extraction.Since the PnP loader is now unconditionally preloaded via
execArgv(seesrc/helpers.tsline 315), the worker no longer needs to extract or register the loader path. This simplifies the worker initialization and aligns with the unified loader approach.
|
Sorry I never noticed this PR previously. Since you're the member of yarn berry project, I believe you know better about PnP surely, so LGTM. Let's see what would happen after merging. |
There was a problem hiding this comment.
Pull request overview
This PR fixes PnP (Plug'n'Play) ESM loader integration by changing how the PnP loader is registered in worker threads. Previously, the code conditionally used module.register() inside the worker thread or --loader in execArgv, which caused a chicken-and-egg problem where the worker couldn't resolve dependencies without the PnP loader, but the loader was only registered after the worker started. The fix ensures the PnP loader is always added via the --loader execArgv flag.
Changes:
- Removed conditional
module.register()call from worker threads that caused initialization issues - Simplified PnP loader setup to always use
--loaderflag in execArgv - Cleaned up unused imports related to module registration
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/index.ts | Removed module.register logic and related imports from runAsWorker function |
| src/helpers.ts | Removed MODULE_REGISTER_SUPPORTED conditional, now always adds --loader flag for PnP |
| .changeset/honest-forks-juggle.md | Added changeset documenting the PnP ESM support fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When running in PnP context,
setupTsRunnerdetects whether the runtime supportmodule.register, and usesmodule.registerin the worker thread to load the PnP loader in the worker thread duringrunAsWoker. If it is not supported,--loader </path/to/.pnp.loader.mjs>is added to the worker'sexecArgv.This is problematic for two reasons:
synckitwithout the PnP loader, but the PnP loader is only loaded inrunAsWorkerwhich needssynckitto be resolvable.This PR makes it so that the
execArgv--loadermethod is always usedFixes #254
Summary by CodeRabbit