Skip to content

src: fix GetSourceCode for ESM file url#296

Closed
santigimeno wants to merge 1 commit intonode-v22.x-nsolid-v5.xfrom
santi/fix_source_code
Closed

src: fix GetSourceCode for ESM file url#296
santigimeno wants to merge 1 commit intonode-v22.x-nsolid-v5.xfrom
santi/fix_source_code

Conversation

@santigimeno
Copy link
Copy Markdown
Member

@santigimeno santigimeno commented Apr 28, 2025

As the loaded modules are indexed by url, we need to convert those into actual paths in order to be able to read those files.

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of file URL and filesystem path conversions to ensure more robust and accurate source code retrieval and testing.
    • Enhanced reliability of dynamic imports by using file URLs instead of plain string paths.
  • Tests

    • Updated tests to use standardized file URL conversions, ensuring consistency and correctness in path comparisons across test cases.

As the loaded modules are indexed by url, we need to convert those into
actual paths in order to be able to read those files.
@santigimeno santigimeno self-assigned this Apr 28, 2025
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 28, 2025

Walkthrough

The changes update both source and test code to handle file path and file URL conversions more robustly by leveraging Node.js utilities and URL parsing libraries. In the main source, the conversion from file URLs to filesystem paths is now performed using ada::parse and node::url::FileURLToPath instead of manual string operations. Correspondingly, the test code replaces direct string manipulations with pathToFileURL and fileURLToPath for accurate path comparisons and imports. An unnecessary debug statement is also removed, and dynamic imports now use file URLs for consistency.

Changes

Files/Group Change Summary
src/nsolid/nsolid_api.cc Added #include "node_url.h", replaced manual file URL parsing with ada::parse and FileURLToPath, removed debug fprintf.
test/agents/test-grpc-source-code.mjs Replaced manual path manipulations with fileURLToPath and pathToFileURL for all relevant test cases and comparisons.
test/common/nsolid-grpc-agent/client.js Updated dynamic import to use pathToFileURL for converting string to file URL before importing.

Poem

In the warren of code, we hop and we twirl,
Paths and URLs now properly unfurl.
No more slicing strings with a nervous twitch—
We parse and convert, each bug we outwitch!
With imports and tests now robust and bright,
The code-bunnies sleep well tonight.
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e76de7a and 58dbfa0.

📒 Files selected for processing (3)
  • src/nsolid/nsolid_api.cc (2 hunks)
  • test/agents/test-grpc-source-code.mjs (5 hunks)
  • test/common/nsolid-grpc-agent/client.js (2 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/nsolid/nsolid_api.cc

[error] 17-17: There is an unknown macro here somewhere. Configuration is required. If SERIALIZABLE_OBJECT_METHODS is a macro then please configure it.

(unknownMacro)

🔇 Additional comments (9)
test/common/nsolid-grpc-agent/client.js (2)

5-5: Good addition of the URL module import.

The import of pathToFileURL is correctly added to support the file URL conversion in handleImport.


85-85: Improved ES module import handling.

Using pathToFileURL to convert filesystem paths to proper file URLs ensures that dynamic imports work correctly with ES Modules, which expect URL format for imports rather than file paths. This change aligns with the broader improvements in URL handling throughout the codebase.

src/nsolid/nsolid_api.cc (2)

17-17: Good addition of the URL module include.

The inclusion of the node_url.h header is necessary for using the new FileURLToPath function.

🧰 Tools
🪛 Cppcheck (2.10-2)

[error] 17-17: There is an unknown macro here somewhere. Configuration is required. If SERIALIZABLE_OBJECT_METHODS is a macro then please configure it.

(unknownMacro)


798-805: Improved ESM file URL handling with proper URL parsing.

The revised implementation properly handles file URL to path conversion using ada::parse and node::url::FileURLToPath instead of manual string manipulation. This approach is more robust as it correctly handles URL encoding, platform-specific path issues, and other edge cases that can occur in file URLs.

The added error checking is also good practice, returning UV_ENOENT if the URL cannot be converted to a valid path.

test/agents/test-grpc-source-code.mjs (5)

7-7: Good addition of URL utility imports.

Adding the pathToFileURL and fileURLToPath imports from the Node.js URL module is necessary to support the more robust URL handling in the tests.


40-41: Improved file URL to path conversion.

Replacing manual string manipulation with fileURLToPath provides more robust handling of file URLs, accounting for URL encoding and platform-specific path issues.


84-86: Properly converting paths to file URLs for consistent comparisons.

Converting file paths to their URL representations using pathToFileURL() and then comparing against frame script names ensures consistent matching behavior across platforms. This aligns with how Node.js handles ESM module specifiers internally.

Also applies to: 93-95


196-198: Consistent path handling in worker thread tests.

The same URL conversion improvements are correctly applied to the worker thread test case, maintaining consistency across the test suite.

Also applies to: 205-207


291-292: Consistent URL handling for data URL tests.

The data URL test case is also updated to use the same path to URL conversion approach, ensuring consistency in how file paths are handled across all test scenarios.

Also applies to: 298-299

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

santigimeno added a commit that referenced this pull request Apr 29, 2025
As the loaded modules are indexed by url, we need to convert those into
actual paths in order to be able to read those files.

PR-URL: #296
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@santigimeno
Copy link
Copy Markdown
Member Author

Landed in 08f118f

@santigimeno santigimeno deleted the santi/fix_source_code branch April 29, 2025 14:35
santigimeno added a commit that referenced this pull request May 1, 2025
As the loaded modules are indexed by url, we need to convert those into
actual paths in order to be able to read those files.

PR-URL: #296
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
santigimeno added a commit that referenced this pull request May 7, 2025
As the loaded modules are indexed by url, we need to convert those into
actual paths in order to be able to read those files.

PR-URL: #296
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants