[New] give resolvers access to parserServices #2238
Conversation
| modulePath, | ||
| sourceFile, | ||
| config, | ||
| context.parserServices, |
There was a problem hiding this comment.
Pass context.parserServices -> resolver.resolve().
|
I guess it seems fine, but we’d need tests, and also what’s the use case? Does any parser actually provide info here, and does any resolver look for it? |
ljharb
added
package: utils
| @@ -80,19 +80,20 @@ exports.fileExistsWithCaseSync = function fileExistsWithCaseSync(filepath, cache | |||
| return result; | |||
| }; | |||
|
|
|||
| function relative(modulePath, sourceFile, settings) { | |||
| return fullResolve(modulePath, sourceFile, settings).path; | |||
| function relative(modulePath, sourceFile, context) { | |||
There was a problem hiding this comment.
this is actually a breaking change for the utils package, so that seems quite suboptimal
There was a problem hiding this comment.
Can I resolve that by accepting either the new value (context) or the old (settings), as in the latest commit?
| modulePath, | ||
| sourceFile, | ||
| // Accept context or settings, for backward compatibility | ||
| !('settings' in context) ? { settings: context } : context, |
There was a problem hiding this comment.
I’m pretty skeptical there’s always a “settings” in context, consistently from eslint 2 - 7.
There was a problem hiding this comment.
Well, we pass context.settings -> it today, so we already rely on that, don't we?
eslint-plugin-import/utils/resolve.js
Line 220 in dd81424
eslint-plugin-import/src/ExportMap.js
Line 434 in dd81424
There was a problem hiding this comment.
i guess that's true; if it wasn't an object it'd throw since it's not checked for truthiness. Still, is there a more robust way to check? perhaps eslint provides a Context constructor or something we could use to compare it to?
There was a problem hiding this comment.
Good question ... Here's where it's constructed today, I think: https://github.com/eslint/eslint/blob/cf34e5cf5ed5d09eb53c16cca06821c4e34b7b70/lib/linter/linter.js#L910-L954
Looking at that, I'm not sure what would be more robust, and consistent between ESLint versions?
There was a problem hiding this comment.
Another alternative is to always pass settings, and just also pass context? We could also throw if settings !== context.settings when context is provided
There was a problem hiding this comment.
Totally -- I propose overloading the argument because I think it's functionally equivalent, and slightly tidier than calling relative(modulePath, sourceFile, context.settings, context), but no strong feelings either way ...
There was a problem hiding this comment.
it's definitely tidier - we could also allow relative(modulePath, sourceFile, null, context), and adapt it later whenever we do a semver-major
Codecov Report
@@ Coverage Diff @@
## main #2238 +/- ##
==========================================
- Coverage 95.23% 94.63% -0.60%
==========================================
Files 65 65
Lines 2686 2685 -1
Branches 888 889 +1
==========================================
- Hits 2558 2541 -17
- Misses 128 144 +16
Continue to review full report at Codecov.
|
Is there a way for resolvers to access parserServices? If not, what do you think about enabling it, maybe like so?