fix: pnp issue when used in a monorepo #390
Conversation
🦋 Changeset detectedLatest commit: 4b1c73b 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 |
WalkthroughThis change updates module resolution logic to improve Plug'n'Play (PnP) compatibility in monorepo setups. It modifies how modules and parsers are required, always using the file being linted as the resolution context, and introduces new test fixtures and configuration files to validate this behavior in a workspace environment. Changes
Sequence Diagram(s)sequenceDiagram
participant Linter
participant ModuleRequire
participant Parser
participant FileSystem
Linter->>ModuleRequire: moduleRequire(parserPath, sourceFile)
ModuleRequire->>FileSystem: Try require relative to eslint
alt fails
ModuleRequire->>FileSystem: Try require relative to main entry
alt fails
ModuleRequire->>FileSystem: Try require relative to sourceFile
alt fails
ModuleRequire->>FileSystem: Fallback to cjsRequire
end
end
end
ModuleRequire-->>Linter: parser module
Linter->>Parser: Parse file with resolved parser
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/utils/parse.tsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js src/utils/module-require.tsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js test/fixtures/yarn-pnp/index.jsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ Context from checks skipped due to timeout of 90000ms (21)
🔇 Additional comments (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
|
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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #390 +/- ##
=======================================
Coverage 96.09% 96.10%
=======================================
Files 96 96
Lines 4944 4951 +7
Branches 1854 1857 +3
=======================================
+ Hits 4751 4758 +7
Misses 192 192
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
commit: |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 7676f9c in 2 minutes and 3 seconds. Click for details.
- Reviewed
280lines of code in9files - Skipped
2files when reviewing. - Skipped posting
4draft 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. src/rules/no-unresolved.ts:71
- Draft comment:
Bypassing the case‐sensitive check when PnP is active. Ensure that skipping case validation in PnP mode is the intended behavior. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that skipping case validation in PnP mode is intended. This falls under the rule of not asking the author to confirm their intention or ensure behavior. Therefore, this comment should be removed.
2. src/utils/module-require.ts:14
- Draft comment:
New parameter 'sourceFile' is added to moduleRequire and later used with createRequire for fallback. This helps resolve modules relative to the linted file in monorepo/PnP setups. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining the purpose of a new parameter. It does not provide a suggestion, ask for confirmation, or point out a potential issue.
3. src/utils/module-require.ts:29
- Draft comment:
A fallback using cjsRequire.main!.require(p) is added. Note the use of the non-null assertion on main; ensure that require.main is always defined in the intended environments. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. src/utils/parse.ts:120
- Draft comment:
Passing context.physicalFilename to moduleRequire ensures the parser is resolved relative to the file being linted. This aligns with the monorepo/PnP fix. - 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 or ask for clarification on the code itself.
Workflow ID: wflow_zUmAOvLvLGYIeOeU
You can customize 
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 6ed2092 in 1 minute and 38 seconds. Click for details.
- Reviewed
58lines 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. src/rules/no-unresolved.ts:70
- Draft comment:
Removed the check for process.versions.pnp. Confirm that always checking case sensitivity (even in PnP) is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/utils/resolve.ts:78
- Draft comment:
The new 'leaf' parameter and nested ternary in fileExistsWithCaseSync are subtle. Verify the logic: if no exact match and not a leaf, the check returns true when no case‐insensitive match is found, which may be inverted. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. test/fixtures/yarn-pnp/index.js:5
- Draft comment:
Test changes import an additional module (add2); consider adding assertions to validate behavior rather than only using console.log. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
Workflow ID: wflow_wX9nd2Xpiv5yicrQ
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 09b61e6 in 53 seconds. Click for details.
- Reviewed
30lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft 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/poor-dolls-move.md:5
- Draft comment:
Changeset file looks good; the version and fix message match the PR title. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. src/utils/resolve.ts:80
- Draft comment:
Refactored file existence check now uses Array.some with a proper case‐insensitive comparison. Verify that comparing both sides in lowercase meets the intended behavior; consider rewriting nested ternaries for clarity and add tests covering case variations. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_PWvEr0O9LX3HxmQe
You can customize
Always use the file being linted as the "location" of the require when loading a resolver for the file. This prevents an issue when the sourceFile is in a dependency of the package being linted. Add a fallback when loading a parser, the fallback uses the file being linted as the location of the require. Using the file being linted isn't perfect because a user might configure the linter to lint files in other packages. However the approach is better than what was before.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 4b1c73b in 54 seconds. Click for details.
- Reviewed
43lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2draft 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. src/utils/resolve.ts:80
- Draft comment:
Improvement: The revised check now correctly lowercases both values. Consider caching parsedPath.base.toLowerCase() to avoid repeated computation over large directory listings. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. test/fixtures/yarn-pnp/index.js:5
- Draft comment:
Minor style: Missing semicolon at end of import statement; ensure consistency with project style. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
Workflow ID: wflow_KJYJMfSpzcKlXTqa
You can customize
Always use the file being linted as the "location" of the require when
loading a resolver for the file. This prevents an issue when the sourceFile
is in a dependency of the package being linted.
Add a fallback when loading a parser, the fallback uses the file being
linted as the location of the require.
Using the file being linted isn't perfect because a user might configure
the linter to lint files in other packages. However the approach is better
than what was before.
close #384
Important
Fixes Plug'n'Play issue in monorepos by improving module resolution and adding test configurations.
moduleRequireinmodule-require.tsandparse()inparse.ts.context.physicalFilenameinparse.ts.resolve.tsto handle multiple source files and improve case sensitivity handling.test-packagewith ESLint and TypeScript configurations inyarn-pnpfixture.yarn-pnpto Yarn 4.9.2 and ESLint 9.29.0.This description was created by
for 4b1c73b. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation