-
-
Couldn't load subscription status.
- Fork 28
feat: improve shareable config #208
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Maybe this is can be a feature instead a fix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves the shareable config functionality by making the system more robust when handling missing or invalid packages, and adds support for package tags. The key changes include switching from npm CLI commands to direct registry API calls and implementing proper error handling.
Key changes:
- Replaces npm CLI dependency with direct npm registry API calls for fetching peer dependencies
- Implements proper error handling that throws exceptions for missing packages instead of silently returning null
- Adds support for package tags using semver resolution
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/utils/npm-utils.js | Refactored fetchPeerDependencies to use npm registry API directly with proper error handling and semver tag support |
| lib/config-generator.js | Updated to handle the new error-throwing behavior and extract package names properly |
| tests/utils/npm-utils.spec.js | Updated tests to reflect the new API-based approach and error-throwing behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| `Version "${version}" not found for package "${name}".` | ||
| ); | ||
| } | ||
| return Object.entries(Object(packageVersion.peerDependencies)).map( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why Object() is used here? Is this to convert packageVersion.peerDependencies to {} when it is undefined?
tests/utils/npm-utils.spec.js
Outdated
| stub.resolves(mockResponse); | ||
|
|
||
| assert.isNull(peerDependencies); | ||
| expect(async () => await fetchPeerDependencies("desired-package")).rejects.toThrowError(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use assert.throws() here? For consistency - the other tests are all assert-style.
|
@aladdin-add, this is ok? |
|
@aladdin-add, ping |
lib/utils/npm-utils.js
Outdated
|
|
||
| const error = npmProcess.error; | ||
| try { | ||
| // eslint-disable-next-line n/no-unsupported-features/node-builtins -- Fallback using built-in fetch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment needs an update. :)
|
Maybe i should add test for package with version tag. |
👍 |
|
when running PS D:\repos\eslint-org\create-config> node .\bin\create-config.js --config eslint-config-eslint-x
@eslint/create-config: v1.10.0
file:///D:/repos/eslint-org/create-config/bin/create-config.js:24
throw error;
^
Error: Cannot fetch "eslint-config-eslint-x@latest" with error: Unexpected Error when fetching package `e
slint-config-eslint-x`: Not found at fetchPeerDependencies (file:///D:/repos/eslint-org/create-config/lib/utils/npm-utils.js:114:15)
at process.processTicksAndRejections (node:internal/process/task_queues:103:5)
at async ConfigGenerator.calc (file:///D:/repos/eslint-org/create-config/lib/config-generator.js:295:
27) at async file:///D:/repos/eslint-org/create-config/bin/create-config.js:52:5
Node.js v25.0.0
the error message seems a bit strange. |
Maybe you're right, so what the Error message we should throw? |
Cannot fetch "eslint-config-eslint-x@latest" with error: Unexpected Error when fetching package `e
slint-config-eslint-x`maybe we could revert the change: 122699f? |
|
could you also checkout some unresolved comments, otherwise LGTM, thanks! 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 LGTM, thanks! leaving it open for 2~3 days for others to review.
Prerequisites checklist
What is the purpose of this pull request?
Improve shareable config (
--configflag). Throw if not found, and can use package tag (e.g.my-eslint-config@alpha).What changes did you make? (Give an overview)
Example:
esl-con-recBefore:
After:
Related Issues
N/A
Is there anything you'd like reviewers to focus on?
N/A