-
Notifications
You must be signed in to change notification settings - Fork 19
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
Manually add .js extension to relative imports #1455
Conversation
WalkthroughWalkthroughThe modifications across various files in the CLI workspace primarily involve updating import statements to include Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 0
Out of diff range and nitpick comments (3)
cli/src/listener.ts (1)
Line range hint
129-129
: Specify a more appropriate type instead ofany
.Using
any
type can lead to potential runtime errors due to lack of type safety. Consider specifying a more detailed type forprintResult
.- const printResult: any[] = [] + const printResult: Array<{ [key: string]: string }> = []cli/src/reporter.ts (1)
Line range hint
153-153
: Specify a more appropriate type instead ofany
.To enhance type safety and prevent potential runtime issues, replace the
any
type with a more specific type.- const printResult: any[] = [] + const printResult: Array<{ [key: string]: string }> = []cli/src/delegator.ts (1)
Line range hint
302-302
: Specify a more appropriate type instead ofany
.Using
any
type reduces type safety, which can lead to bugs. It's recommended to use a more specific type to ensure the integrity of the data being handled.- const printResult: any[] = [] + const printResult: Array<{ [key: string]: string }> = []
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
cli/package.json
is excluded by!**/*.json
yarn.lock
is excluded by!**/*.lock
,!**/*.lock
Files selected for processing (16)
- .vscode/extensions.json (1 hunks)
- cli/.eslintrc (1 hunks)
- cli/src/adapter.ts (1 hunks)
- cli/src/aggregator.ts (1 hunks)
- cli/src/chain.ts (1 hunks)
- cli/src/cli-types.ts (1 hunks)
- cli/src/datafeed.ts (3 hunks)
- cli/src/delegator.ts (1 hunks)
- cli/src/fetcher.ts (1 hunks)
- cli/src/index.ts (1 hunks)
- cli/src/listener.ts (1 hunks)
- cli/src/proxy.ts (1 hunks)
- cli/src/reporter.ts (1 hunks)
- cli/src/service.ts (1 hunks)
- cli/src/utils.ts (1 hunks)
- cli/src/vrf.ts (1 hunks)
Files skipped from review due to trivial changes (12)
- .vscode/extensions.json
- cli/src/adapter.ts
- cli/src/aggregator.ts
- cli/src/chain.ts
- cli/src/cli-types.ts
- cli/src/datafeed.ts
- cli/src/fetcher.ts
- cli/src/index.ts
- cli/src/proxy.ts
- cli/src/service.ts
- cli/src/utils.ts
- cli/src/vrf.ts
Additional Context Used
GitHub Check Runs (1)
build-publish success (5)
cli/src/delegator.ts: [warning] 302-302:
Unexpected any. Specify a different type
cli/src/listener.ts: [warning] 129-129:
Unexpected any. Specify a different type
cli/src/reporter.ts: [warning] 153-153:
Unexpected any. Specify a different type
Additional comments not posted (6)
cli/.eslintrc (3)
12-13
: Addition of@typescript-eslint
andimport
plugins to ESLint configuration.This ensures TypeScript-specific linting and import rules are enforced, improving code quality and consistency.
20-28
: Configuration of TypeScript-specific and import extension rules.Disabling TypeScript-specific lint warnings (
@typescript-eslint/ban-ts-comment
) and enforcing import extensions for.js
and.ts
files enhances code reliability and module resolution correctness.
30-37
: Override settings for test directories to disable import extension rules.This is a sensible exception, as test environments often have different module resolution configurations. It prevents unnecessary linting errors in tests.
cli/src/listener.ts (1)
3-11
: Update of import statements to include.js
extensions.This change is crucial for correct module resolution in Node.js environments when using ES modules, aligning with the PR's objectives.
cli/src/reporter.ts (1)
10-12
: Update of import statements to include.js
extensions.Ensures that the module resolution works correctly in a Node.js environment, which is critical for the functionality of the CLI tool.
cli/src/delegator.ts (1)
3-4
: Update of import statements to include.js
extensions.Correctly resolves modules in ES module environments, crucial for the operation of the CLI tool.
In which situation were I haven't been experiencing |
thanks for the feedback Nick. in "bin": {
"cli": "dist/index.js"
} we also explain in the documentation that users can call the cli command globally with |
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! I didn't know that the document was recommending to use binary file, let's merge it
Closes #1454
Description
We are using ES modules in the
cli
workspace and currently, most if not all of our imports in the cli workspace don't include the file extension, which is a requirement in ES modules. Since tsc doesn't add .js extension during compilation, there is amodule not found
error when we try to runnode dist/index.js
.This PR:
.js
extensions to all importseslint-plugin-import
package to provide linting error suggestions when imported file extension is not provided (ignores the test dir)I've tested running the cli executable after these implementations and the
module not found
error has been resolved.Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment