fix: pnp issue when used in a monorepo #384
Conversation
|
|
Warning Rate limit exceeded@JounQin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe changes update the Changes
Poem
✨ 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 (
|
|
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. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to c932f40 in 1 minute and 35 seconds. Click for details.
- Reviewed
66lines of code in3files - Skipped
0files when reviewing. - Skipped posting
5draft 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/module-require.ts:15
- Draft comment:
New 'context' parameter added to moduleRequire. Ensure that both ChildContext and RuleContext reliably provide a valid 'physicalFilename'. - 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 certain contexts provide a validphysicalFilename. This falls under the category of asking the author to ensure something, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue, making it unapproved.
2. src/utils/module-require.ts:41
- Draft comment:
Using createRequire(context.physicalFilename) to load modules relative to the linted file is appropriate. Verify that 'physicalFilename' always points to the correct file in all contexts. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to verify something, which is against the rules. It does not provide a specific suggestion or point out a specific issue.
3. src/utils/module-require.ts:29
- Draft comment:
Empty catch blocks are used in moduleRequire to fallback silently. Consider logging errors for debugging if resolution unexpectedly fails. - 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' to moduleRequire ensures the parser is resolved relative to the linted file, which aligns with the monorepo 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 certain approach is taken. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue.
5. src/utils/resolve.ts:303
- Draft comment:
Replacing 'sourceFile' with 'context.physicalFilename' in normalizeConfigResolvers ensures resolvers load relative to the actual file being linted, which improves monorepo resolution. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining the reason behind a code change. It does not provide a suggestion, ask for confirmation, or point out a potential issue.
Workflow ID: wflow_dn8QP1VGh1hqzXHz
You can customize 
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/utils/module-require.ts(3 hunks)src/utils/parse.ts(1 hunks)src/utils/resolve.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/utils/parse.ts (1)
src/utils/module-require.ts (1)
moduleRequire(15-48)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (3)
src/utils/module-require.ts (1)
15-18:✅ Verification successful
Signature change is breaking – make sure every call-site is patched
moduleRequirenow requires a secondcontextargument. Any forgotten call will turn the compile error into a runtimeundefined is not a functionthe moment TS is bypassed (JS build, tests, etc.).
I’ve spotted the update inparse.ts, but please run a quick grep to confirm nothing else slipped through.
🏁 Script executed:
#!/bin/bash # List invocations that still pass a single argument rg --no-heading -n "moduleRequire\\s*<[^>]*>?\\s*\\([^,)]*\\)" | grep -v "module-require.ts"Length of output: 88
All
moduleRequirecall-sites updatedA search for single-argument invocations returned no results outside
module-require.ts. The new two-parameter signature has been applied everywhere.src/utils/parse.ts (1)
120-121: Call-site updated correctly – LGTMThe extra
contextargument matches the newmoduleRequiresignature. No other logic affected.src/utils/resolve.ts (1)
303-304: Switching tocontext.physicalFilenamefixes PnP, but guard againstundefined
normalizeConfigResolversnow receivescontext.physicalFilename; great for Yarn-PnP look-ups.
However, when ESLint feeds virtual filenames this will becomeundefined, changing the previous behaviour (where a concretesourceFilewas guaranteed). Please double-check the implementation ofnormalizeConfigResolversto ensure it copes withundefined, or add a fallback:- normalizeConfigResolvers( - configResolvers, - context.physicalFilename, + normalizeConfigResolvers( + configResolvers, + context.physicalFilename ?? sourceFile, )
|
It seems depending on undefined behavior of yarn pnp. |
|
@JounQin which behavior is it depending on that is undefined? It is just calling createRequire like the code has already been doing.
|
|
@arcanis Do you think is this a reasonable and intended solution for Yarn P'n'P? |
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.
commit: |
|
@scytacki After thinking days, I'm fine with current changes now. Would you like to add a new e2e test case based on https://github.com/un-ts/eslint-plugin-import-x/tree/master/test/fixtures/yarn-pnp and https://github.com/un-ts/eslint-plugin-import-x/blob/master/test/fixtures.spec.ts? Or I can do it myself. cc @SukkaW |
| @@ -117,7 +117,7 @@ export function parse( | |||
| // require the parser relative to the main module (i.e., ESLint) | |||
| const parser = | |||
| typeof parserOrPath === 'string' | |||
| ? moduleRequire<TSESLint.Parser.ParserModule>(parserOrPath) | |||
| ? moduleRequire<TSESLint.Parser.ParserModule>(parserOrPath, context) | |||
There was a problem hiding this comment.
| ? moduleRequire<TSESLint.Parser.ParserModule>(parserOrPath, context) | |
| ? moduleRequire<TSESLint.Parser.ParserModule>(parserOrPath, context.physicalFilname) |
What about only passing context.physicalFilname instead of passing the entire context here?
I don't see any reason why moduleRequire needs to accept the entire context. Let's keep the utility minimum and maintain the semantics.
There was a problem hiding this comment.
I was hoping that eslint would eventually add the path of the active config file to the context. So by passing the context we'd only have to update the moduleRequire implementation instead of the places that call it.
I could also see eslint considering adding a function like moduleRequire to the context itself. Basically meaning: "given the current context require this module".
So with those cases it seems to make sense to use the context as the "context" of the require, rather than the path of the file being linted.
There was a problem hiding this comment.
I could also see eslint considering adding a function like moduleRequire to the context itself. Basically meaning: "given the current context require this module".
Is there any related reference? I don't think this still need to be required in favor of flat config?
There was a problem hiding this comment.
No reference, that is just my wishful thinking. I assume there will continue to be plugins even with the flat config which use strings to dynamically load optional dependencies (like what import-x is doing). So it would be a nice addition to eslint to make this easier for plugins to do in a way that is better for strict module loaders like Yarn PnP.
But I haven't done research on this. Perhaps eslint really wants to encourage people to stop using strings to load optional dependencies, in that case they will be unlikely to add this kind of method to the context.
There was a problem hiding this comment.
Then let's not assume the changes on context, I think sourceFile: string is more reasonable here.
Don't forget to add the related e2e test case, thanks!
|
@JounQin if you can add that e2e test that'd be great. If I do it, it will take me a few days to find time again. |
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.
All of the existing tests are passing. I don't time to figure out how to add a test specific to this change. I think the test code would have to be run in a PnP repo that has the top level fallback disabled.