Fix: wrong comment when module not found in config (fixes #8192) #8196
Conversation
|
LGTM |
|
@alberto, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas, @gyandeeps and @mysticatea to be potential reviewers. |
alberto
added
accepted
You can detect module-loading errors by checking Could you clarify what errors this logic is checking for? Intuitively, it seems like we should be able to avoid throwing TypeErrors, so I'm confused what the effect of catching them is. Also, I'm not sure I understand why this logic is only catching missing-module errors related to |
|
LGTM |
|
I have updated the PR, I hope the intention is clearer now. I tried to provide a better error for people trying to load an inexistent config from a shared config, a plugin or one of the eslint core configs. Main use case is someone has a typo in his config (e.g. |
|
LGTM |
lib/config/config-file.js
Outdated
| try { | ||
| if (parentPath.indexOf("eslint:") === 0) { |
There was a problem hiding this comment.
Nitpick: parentPath.startsWith("eslint:") might be a bit more readable.
lib/config/config-file.js
Outdated
| */ | ||
| parentPath = (!path.isAbsolute(parentPath) | ||
| ? path.join(relativeTo || path.dirname(filePath), parentPath) | ||
| : parentPath |
There was a problem hiding this comment.
It might be more readable to avoid negating the condition here.
parentPath = path.isAbsolute(parentPath)
? parentPath
: path.join(relativeTo || path.dirname(filePath), parentPath);There was a problem hiding this comment.
I didn't really touch this part, but, yeah, I will update it while I am on it
lib/config/config-file.js
Outdated
| const packagePath = filePath.substr(7, filePath.lastIndexOf("/") - 7); | ||
| const configName = filePath.substr(filePath.lastIndexOf("/") + 1, filePath.length - filePath.lastIndexOf("/") - 1); | ||
|
|
||
| normalizedPackageName = normalizePackageName(packagePath, "eslint-plugin"); | ||
| debug(`Attempting to resolve ${normalizedPackageName}`); | ||
| filePath = resolver.resolve(normalizedPackageName, getLookupPath(relativeTo)); | ||
| return { filePath, configName }; | ||
| return { filePath, configName, pluginPath }; |
There was a problem hiding this comment.
Is there a reason to include pluginPath in the return value when it's always the same as filePath?
According to the JSDoc comment, the return value of this function is "A path that can be used directly to load the configuration", but it would be useful if it described the properties of the return value in more detail.
There was a problem hiding this comment.
pluginPath is not a very accurate name. Maybe configFullName is more appropriate. Its value is something like plugin:node/recommended, which is what I wanted to include in the error message, since it is what the user will have in its config file.
filePath is modified in the previous line to contain the resolved path of the config file.
I'm not very fond of JSDoc. How should I document the optional properties of a return object?
There was a problem hiding this comment.
I'm not sure if there's an official JSDoc way to do it, but just explaining the optional properties in words would be a good improvement.
|
LGTM |
|
LGTM |
|
LGTM |
|
LGTM |
|
@not-an-aardvark let me know if it LGTY now. Thanks for your feedback, I'm much happier with the code now :) |
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
PR #8100 modified the message when there was an error loading a plugin or config, but didn't account for other possible errors. In this case,
babel-eslintwas not present, and the error was misleading.Is there anything you'd like reviewers to focus on?
Is there a cleaner way to check for this rather than using the error message?