-
Notifications
You must be signed in to change notification settings - Fork 193
fix: fix composition wrapper by adding URL.canParse to polyfill #2115
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
WalkthroughA Makefile was introduced for running tests in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.6)composition-go/index.global.jsNote ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Router image scan passed✅ No security vulnerabilities found in image: |
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: 2
🧹 Nitpick comments (1)
composition-go/composition_test.go (1)
38-38: LGTM! Appropriate test data for URL functionality.The addition of well-formed HTTPS URLs to the subgraph definitions supports the new URL parsing capabilities.
Consider adding explicit tests for the new
URL.canParsefunctionality to ensure it works correctly with both valid and invalid URLs. Would you like me to help generate test cases for this?Also applies to: 62-62
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
composition-go/Makefile(1 hunks)composition-go/composition_test.go(2 hunks)composition-go/index.global.js(1 hunks)composition-go/polyfill.go(1 hunks)composition-go/shim/src/polyfill.js(1 hunks)composition-go/vm_goja.go(1 hunks)composition-go/vm_v8.go(2 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
composition-go/Makefile
[warning] 5-5: Missing required phony target "all"
(minphony)
[warning] 5-5: Missing required phony target "clean"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
composition-go/shim/src/polyfill.js (1)
14-16: LGTM! Clean implementation of URL.canParse polyfill.The static method implementation correctly delegates to the runtime-provided
urlCanParsefunction, maintaining consistency with the existingurlParsepattern.composition-go/Makefile (1)
1-5: LGTM! Focused test configuration for both VM implementations.The Makefile correctly tests both the default (Goja) and V8 implementations with appropriate verbose output. The static analysis warnings about missing "all" and "clean" targets are acceptable for this focused testing Makefile.
composition-go/vm_goja.go (1)
79-81: LGTM! Consistent runtime binding implementation.The
urlCanParsebinding follows the established pattern for runtime function exposure and includes proper error handling.composition-go/vm_v8.go (2)
242-253: LGTM! Correct V8 wrapper function implementation.The
urlCanParseV8function properly extracts the URL argument, calls the GourlCanParsefunction, and handles the boolean result conversion to V8 value with appropriate error handling.
270-273: LGTM! Proper V8 binding registration.The
urlCanParsefunction template is correctly registered as a read-only global function, following the established pattern for other runtime bindings.
Aenimus
left a comment
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!
Summary by CodeRabbit
Checklist