feat: add local-dependency rule #11
Conversation
|
Hi @zetlen! Happy new year! Any chance you can take a look at this. |
lib/rules/local-dependency.js
Outdated
| // Rule Definition | ||
| //------------------------------------------------------------------------------ | ||
|
|
||
| function fileExistsWithCaseSync(filepath) { |
There was a problem hiding this comment.
[Suggestion] Resolving a path to a package can be pretty tricky 😄. I would think require.resolve would work instead of the body of this function, to avoid a recursive call. Have you tried that?
Note that ESM environments might need https://nodejs.org/api/module.html#modulecreaterequirefilename...
JoshuaKGoldberg
added
the
status: waiting for author
Co-authored-by: Josh Goldberg ✨ <[email protected]>
| ); | ||
|
|
||
| try { | ||
| if (!require.resolve(filePath)) { |
There was a problem hiding this comment.
Thanks for the tip @JoshuaKGoldberg! I am not totally following the create resolve comment but it seems like require.resolve is working.
There was a problem hiding this comment.
I was thinking that if & when ESLint supports running rules as ESM, this might crash. But I suppose we can cross that bridge when we get to it.
There was a problem hiding this comment.
At some point Node will support https://nodejs.org/api/esm.html#importmetaresolvespecifier so you could do a forward-compatible feature detection here.
JoshuaKGoldberg
removed
the
status: waiting for author
There was a problem hiding this comment.
Awesome, thanks @kendallgassner! I'm not used to coming across three year old PRs and the original author immediately responding to review 😄 this was wonderful.
Will leave open for a bit in case @zetlen wants to give input. Setting myself a reminder to check back in next week.
package-lock.json
Outdated
| } catch (e) { | ||
| context.report({ | ||
| node: packageRoot, | ||
| message: `The package ${key} does not exist given the specified path: ${value}.` |
There was a problem hiding this comment.
[Thoughts] It'd be nice if it used a different, more explicit message if the only difference is casing. But that's probably better as a follow-up issue. I bet it's surprisingly hard to get right 😄 .
There was a problem hiding this comment.
Technically it will catch spelling mistakes and non-existing packages.... but package managers should catches those so the rule is mostly useful for casing errors.
There was a problem hiding this comment.
Good point. I think I'd personally still interpret it for both, since it gives the nice errors before package managers would.
|
@JoshuaKGoldberg You have my general signoff going forward with this and the rest of the code! Don't hesitate to ask me questions, but don't worry about my rubber stamp. |
Co-authored-by: Josh Goldberg ✨ <[email protected]>
Co-authored-by: Josh Goldberg ✨ <[email protected]>
Co-authored-by: Josh Goldberg ✨ <[email protected]>
Co-authored-by: Josh Goldberg ✨ <[email protected]>
|
Just a heads up @kendallgassner, I changed the license for future versions of the library to MIT in #38. That doesn't impact reviewing this PR at all - I just don't know how changing a license does/should impact existing PRs. |
JoshuaKGoldberg
changed the title
Basically I keep making a mistake where I case am installing a local dependency using "link" and I mess up the casing:
This:
Should be
Npm doesn't support link but yarn does. When the path is wrong yarn does not sub out the link when releasing.