Exported url search params to be usable outside #18
Conversation
WalkthroughThe pull request introduces several enhancements to the Goja runtime, particularly in the Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18 +/- ##
==========================================
+ Coverage 47.20% 53.07% +5.86%
==========================================
Files 15 15
Lines 1684 1692 +8
==========================================
+ Hits 795 898 +103
+ Misses 807 706 -101
- Partials 82 88 +6
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (10)
gojaRuntime/url/nodeurl.go (1)
Line range hint
29-63: Consider adding String() method to satisfy fmt.Stringer interface.While the implementation is correct, consider explicitly declaring the
fmt.Stringerinterface implementation. This would make the interface satisfaction more obvious and provide better documentation.+var _ fmt.Stringer = SearchParams{} type SearchParams []SearchParam🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 13-13: gojaRuntime/url/nodeurl.go#L13
Added line #L13 was not covered by tests
[warning] 21-21: gojaRuntime/url/nodeurl.go#L21
Added line #L21 was not covered by tests
[warning] 23-23: gojaRuntime/url/nodeurl.go#L23
Added line #L23 was not covered by tests
[warning] 25-25: gojaRuntime/url/nodeurl.go#L25
Added line #L25 was not covered by tests
[warning] 31-31: gojaRuntime/url/nodeurl.go#L31
Added line #L31 was not covered by tests
[warning] 35-35: gojaRuntime/url/nodeurl.go#L35
Added line #L35 was not covered by tests
[warning] 39-40: gojaRuntime/url/nodeurl.go#L39-L40
Added lines #L39 - L40 were not covered by tests
[warning] 43-43: gojaRuntime/url/nodeurl.go#L43
Added line #L43 was not covered by testsgojaRuntime/url/url.go (3)
139-139: Consider documenting the exported fields and their intended usage.The change from
urltoUrlmakes these fields public, which aligns with the PR objective. However, to maintain code quality and help other developers:
- Add documentation comments for the exported fields
- Consider adding examples of intended usage
- Document any constraints or assumptions
Also applies to: 216-218, 267-267, 287-288, 320-322
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 139-139: gojaRuntime/url/url.go#L139
Added line #L139 was not covered by tests
244-250: Consider enhancing error handling for URL parsing.The URL parsing error is silently ignored. Consider:
- Logging the error for debugging
- Returning an error to the caller
- Adding validation for the hostname format
if _, err := url.ParseRequestURI(u.Url.Scheme + "://" + h); err == nil { if port := u.Url.Port(); port != "" { u.Url.Host = h + ":" + port } else { u.Url.Host = h } m.fixURL(u.Url) +} else { + // Consider logging the error or returning it + log.Printf("Failed to parse hostname %q: %v", h, err) }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 244-246: gojaRuntime/url/url.go#L244-L246
Added lines #L244 - L246 were not covered by tests
[warning] 248-248: gojaRuntime/url/url.go#L248
Added line #L248 was not covered by tests
[warning] 250-250: gojaRuntime/url/url.go#L250
Added line #L250 was not covered by tests
337-341: Consider optimizing SearchParams initialization.The current implementation creates a new SearchParams slice even when empty. Consider:
- Using a sync.Once for lazy initialization
- Reusing an empty slice constant
- Adding comments explaining the synchronization logic
+var emptySearchParams = make(SearchParams, 0) + if u.SearchParams != nil { u.SearchParams = parseSearchQuery(u.Url.RawQuery) if u.SearchParams == nil { - u.SearchParams = make(SearchParams, 0) + u.SearchParams = emptySearchParams } }gojaRuntime/url/urlsearchparams.go (2)
Line range hint
127-165: Consider optimizing the delete methodWhile the implementation is correct, the delete method could be more efficient by using a slice filtering approach.
p.Set("delete", m.r.ToValue(func(call goja.FunctionCall) goja.Value { u := toUrlSearchParams(m.r, call.This) if len(call.Arguments) < 1 { panic(newMissingArgsError(m.r, `The "name" argument must be specified`)) } name := call.Argument(0).String() - isValid := func(v SearchParam) bool { - if len(call.Arguments) == 1 { - return v.Name != name - } else if v.Name == name { - arg := call.Argument(1) - if !goja.IsUndefined(arg) && v.Value == arg.String() { - return false - } - } - return true - } - j := 0 - for i, v := range u.SearchParams { - if isValid(v) { - if i != j { - u.SearchParams[j] = v - } - j++ - } - } - u.SearchParams = u.SearchParams[:j] + filtered := make([]SearchParam, 0, len(u.SearchParams)) + for _, v := range u.SearchParams { + if len(call.Arguments) == 1 { + if v.Name != name { + filtered = append(filtered, v) + } + } else { + arg := call.Argument(1) + if v.Name != name || (goja.IsUndefined(arg) || v.Value != arg.String()) { + filtered = append(filtered, v) + } + } + } + u.SearchParams = filtered u.markUpdated() return goja.Undefined() }))
262-283: Consider refactoring the set method for better maintainabilityThe set method handles multiple responsibilities and could be split into smaller, more focused functions.
+func (u *UrlSearchParams) replaceFirstValue(name, value string) (int, bool) { + found := false + j := 0 + for i, sp := range u.SearchParams { + if sp.Name == name { + if found { + continue + } + u.SearchParams[i].Value = value + found = true + } + if i != j { + u.SearchParams[j] = sp + } + j++ + } + return j, found +} p.Set("set", m.r.ToValue(func(call goja.FunctionCall) goja.Value { u := toUrlSearchParams(m.r, call.This) if len(call.Arguments) < 2 { panic(newMissingArgsError(m.r, `The "name" and "value" arguments must be specified`)) } name := call.Argument(0).String() value := call.Argument(1).String() - found := false - j := 0 - for i, sp := range u.SearchParams { - if sp.Name == name { - if found { - continue - } - u.SearchParams[i].Value = value - found = true - } - if i != j { - u.SearchParams[j] = sp - } - j++ - } + j, found := u.replaceFirstValue(name, value) if !found { u.SearchParams = append(u.SearchParams, SearchParam{ Name: name, Value: value, }) } else { u.SearchParams = u.SearchParams[:j] } u.markUpdated() return goja.Undefined() }))gojaRuntime/gojaRuntime.go (2)
262-267: Enhance function documentation.While the implementation is correct, consider expanding the documentation to describe:
- The purpose of modifying context before VM setup
- The return value's significance
- Example usage scenarios
Consider updating the comment to:
-// BeforeVMSetupFunc allows to set a function that will be called before the VM is setup. +// BeforeVMSetupFunc sets a hook function that will be called before VM initialization. +// This allows for context modifications that need to be in place before the VM starts. +// The modified context is then used throughout the VM's lifecycle. +// Returns the modified context which will be used for subsequent operations.
318-318: Consider extracting execution duration calculation.While the implementation is correct, consider extracting the execution duration calculation into a separate method for better code organization and reusability.
+func (e *GojaRunnerV1) trackExecutionDuration(result *actionResult) func() { + startedAt := result.RunMetadata.StartedAt + return func() { + result.RunMetadata.ExecutionDuration = time.Since(startedAt) + } +} func (e *GojaRunnerV1) Execute(ctx context.Context, workflow runtimesRegistry.WorkflowDescriptor, startOptions runtimesRegistry.StartOptions) (runtimesRegistry.ExecutionResult, error) { vm := goja.New() ctx = __beforeVmSetupFunc(ctx, vm) executionResult, returnErr := e.setupVM(ctx, vm, workflow, startOptions.Loggger) __afterVmSetupFunc(ctx, vm) if returnErr != nil { return executionResult, returnErr } - defer func(startedAt time.Time) { - executionResult.RunMetadata.ExecutionDuration = time.Since(startedAt) - }(executionResult.RunMetadata.StartedAt) + defer e.trackExecutionDuration(executionResult)()Also applies to: 326-329
runtime_test.go (2)
14-14: Consider renaming the import alias to avoid shadowing 'net/url'The import alias
urlmight cause confusion with Go's standard library packagenet/url. To improve code clarity and prevent potential naming conflicts, consider using a different alias, such asgojaURL.
58-68: Improve readability of embedded JavaScript codeThe embedded JavaScript code in the test is minified and difficult to read. For better maintainability and debugging, consider formatting the code or loading it from an external file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
gojaRuntime/gojaRuntime.go(4 hunks)gojaRuntime/url/nodeurl.go(4 hunks)gojaRuntime/url/url.go(3 hunks)gojaRuntime/url/urlsearchparams.go(13 hunks)runtime_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
gojaRuntime/url/nodeurl.go
[warning] 13-13: gojaRuntime/url/nodeurl.go#L13
Added line #L13 was not covered by tests
[warning] 21-21: gojaRuntime/url/nodeurl.go#L21
Added line #L21 was not covered by tests
[warning] 23-23: gojaRuntime/url/nodeurl.go#L23
Added line #L23 was not covered by tests
[warning] 25-25: gojaRuntime/url/nodeurl.go#L25
Added line #L25 was not covered by tests
[warning] 31-31: gojaRuntime/url/nodeurl.go#L31
Added line #L31 was not covered by tests
[warning] 35-35: gojaRuntime/url/nodeurl.go#L35
Added line #L35 was not covered by tests
[warning] 39-40: gojaRuntime/url/nodeurl.go#L39-L40
Added lines #L39 - L40 were not covered by tests
[warning] 43-43: gojaRuntime/url/nodeurl.go#L43
Added line #L43 was not covered by tests
[warning] 54-54: gojaRuntime/url/nodeurl.go#L54
Added line #L54 was not covered by tests
[warning] 76-76: gojaRuntime/url/nodeurl.go#L76
Added line #L76 was not covered by tests
[warning] 88-90: gojaRuntime/url/nodeurl.go#L88-L90
Added lines #L88 - L90 were not covered by tests
[warning] 97-99: gojaRuntime/url/nodeurl.go#L97-L99
Added lines #L97 - L99 were not covered by tests
[warning] 106-110: gojaRuntime/url/nodeurl.go#L106-L110
Added lines #L106 - L110 were not covered by tests
[warning] 117-120: gojaRuntime/url/nodeurl.go#L117-L120
Added lines #L117 - L120 were not covered by tests
[warning] 127-127: gojaRuntime/url/nodeurl.go#L127
Added line #L127 was not covered by tests
[warning] 141-141: gojaRuntime/url/nodeurl.go#L141
Added line #L141 was not covered by tests
[warning] 143-143: gojaRuntime/url/nodeurl.go#L143
Added line #L143 was not covered by tests
gojaRuntime/url/url.go
[warning] 139-139: gojaRuntime/url/url.go#L139
Added line #L139 was not covered by tests
[warning] 216-218: gojaRuntime/url/url.go#L216-L218
Added lines #L216 - L218 were not covered by tests
[warning] 233-233: gojaRuntime/url/url.go#L233
Added line #L233 was not covered by tests
[warning] 244-246: gojaRuntime/url/url.go#L244-L246
Added lines #L244 - L246 were not covered by tests
[warning] 248-248: gojaRuntime/url/url.go#L248
Added line #L248 was not covered by tests
[warning] 250-250: gojaRuntime/url/url.go#L250
Added line #L250 was not covered by tests
[warning] 258-258: gojaRuntime/url/url.go#L258
Added line #L258 was not covered by tests
[warning] 263-263: gojaRuntime/url/url.go#L263
Added line #L263 was not covered by tests
[warning] 267-267: gojaRuntime/url/url.go#L267
Added line #L267 was not covered by tests
[warning] 273-273: gojaRuntime/url/url.go#L273
Added line #L273 was not covered by tests
[warning] 287-288: gojaRuntime/url/url.go#L287-L288
Added lines #L287 - L288 were not covered by tests
[warning] 295-295: gojaRuntime/url/url.go#L295
Added line #L295 was not covered by tests
[warning] 297-297: gojaRuntime/url/url.go#L297
Added line #L297 was not covered by tests
🔇 Additional comments (13)
gojaRuntime/url/nodeurl.go (4)
8-10: LGTM! Appropriate visibility changes for external usage.
The struct and field names are now properly exported, which aligns with the PR objective of making these types usable outside.
72-85: Add tests for URL synchronization logic.
The URL synchronization methods are critical for maintaining consistency between the URL's raw query and the search parameters. These methods currently lack test coverage.
#!/bin/bash
# Check for existing synchronization tests
rg -l "Test.*syncSearchParams|Test.*rawQueryUpdateNeeded" Would you like me to help generate test cases covering various synchronization scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-76: gojaRuntime/url/nodeurl.go#L76
Added line #L76 was not covered by tests
Line range hint 127-147: Add comprehensive tests for query parsing.
The query parsing logic handles various cases (empty query, missing values, etc.) but lacks test coverage. This is critical functionality that should be thoroughly tested.
#!/bin/bash
# Check for existing parse tests
rg -l "Test.*parseSearchQuery" Would you like me to help generate test cases covering:
- Empty queries
- Malformed parameters
- Special characters
- Multiple parameters
- Missing values
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 141-141: gojaRuntime/url/nodeurl.go#L141
Added line #L141 was not covered by tests
[warning] 143-143: gojaRuntime/url/nodeurl.go#L143
Added line #L143 was not covered by tests
88-124: Consider performance optimization for parameter lookups.
The current implementation performs separate iterations for each lookup operation. For better performance with large parameter sets, consider using a map-based cache for frequently accessed parameters.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 88-90: gojaRuntime/url/nodeurl.go#L88-L90
Added lines #L88 - L90 were not covered by tests
[warning] 97-99: gojaRuntime/url/nodeurl.go#L97-L99
Added lines #L97 - L99 were not covered by tests
[warning] 106-110: gojaRuntime/url/nodeurl.go#L106-L110
Added lines #L106 - L110 were not covered by tests
[warning] 117-120: gojaRuntime/url/nodeurl.go#L117-L120
Added lines #L117 - L120 were not covered by tests
gojaRuntime/url/urlsearchparams.go (5)
13-13: LGTM: Type export follows Go conventions
The renaming of the type to UrlSearchParams follows Go's naming convention for exported types, making it accessible outside the package.
Line range hint 64-109: LGTM: Parameter building logic is sound
The parameter building methods are well-structured with proper error handling and consistent parameter manipulation patterns.
185-190: LGTM: forEach implementation is correct
The forEach implementation properly handles callback arguments and error cases.
316-319: LGTM: markUpdated implementation is correct
The method properly handles URL updates by clearing the RawQuery when necessary.
356-364: LGTM: Iterator implementation follows the spec
The iterator implementation correctly follows the JavaScript iterator protocol for URLSearchParams.
gojaRuntime/gojaRuntime.go (3)
175-176: LGTM: Global function declarations are well-defined.
The new __beforeVmSetupFunc follows the established naming convention and provides a safe default implementation that preserves the input context.
288-288: LGTM: Context modification is properly integrated.
The __beforeVmSetupFunc is correctly placed before VM setup, ensuring any context modifications are available during introspection.
262-267: Implementation aligns well with PR objectives.
The addition of BeforeVMSetupFunc and its integration in both Introspect and Execute methods provides a robust mechanism for making URL search params available outside by allowing context modifications before VM setup.
Let's verify the usage of this new functionality:
Also applies to: 288-288, 318-318
✅ Verification successful
Let me verify the implementation further by checking how the BeforeVMSetupFunc is actually used in the execution paths.
Implementation is correctly integrated and used as intended
The verification confirms that:
BeforeVMSetupFuncis properly integrated in both execution paths (IntrospectandExecute)- The function is called at the right moment - before VM setup and before any code execution
- The modified context is properly passed through the execution chain
- The implementation has a safe default that returns the original context when no custom function is set
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of BeforeVMSetupFunc in relation to URL search params
# Test 1: Check for URL-related imports and usage
echo "Checking for URL-related code..."
rg -l "url" --type go
# Test 2: Look for BeforeVMSetupFunc usage
echo "Checking for BeforeVMSetupFunc usage..."
rg "BeforeVMSetupFunc" --type go
# Test 3: Look for URL search params related code
echo "Checking for URL search params..."
rg -i "urlsearchparams|searchparams" --type go
Length of output: 10645
Script:
#!/bin/bash
# Check the implementation and usage of BeforeVMSetupFunc in execution paths
rg -A 10 "__beforeVmSetupFunc" --type go
Length of output: 2675
runtime_test.go (1)
24-24: Test function name clarity
The test function Test_GojaLogVmInits seems to be testing multiple VM initializations. Ensure that the function name accurately reflects its purpose and follows naming conventions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
gojaRuntime/url/urlsearchparams_test.go (2)
11-110: Consider refactoring tests to use table-driven patterns.The test structure is consistent but could be more maintainable and extensible. Consider using table-driven tests to:
- Reduce code duplication
- Make it easier to add new test cases
- Improve test coverage with edge cases
Here's a suggested refactor pattern:
func TestURLSearchParams(t *testing.T) { tests := []struct { name string script string expected string }{ { name: "append", script: `const params = new URLSearchParams(); params.append("name", "value"); params.get("name");`, expected: "value", }, // Add more test cases here } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { vm := createVM() result, err := vm.RunString(tt.script) if err != nil { t.Fatalf("Failed to process script for %s: %v", tt.name, err) } if result.String() != tt.expected { t.Errorf("Expected '%s', got '%s'", tt.expected, result.String()) } }) } }
20-23: Enhance error handling with more descriptive messages.The current error handling pattern uses generic messages. Consider:
- Including the specific operation that failed
- Adding cleanup in case of errors
- Using
t.Cleanup()for VM resourcesExample improvement:
- if err != nil { - t.Fatal("Failed to process url script.", err) - } + if err != nil { + t.Fatalf("Failed to execute URLSearchParams operation '%s': %v", + "append", err) + } + t.Cleanup(func() { + // Cleanup VM resources if needed + })Also applies to: 40-43, 60-63, 80-83, 101-104
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
gojaRuntime/url/urlsearchparams_test.go(1 hunks)
🔇 Additional comments (2)
gojaRuntime/url/urlsearchparams_test.go (2)
11-110: 🛠️ Refactor suggestion
Add test cases for edge cases and special characters.
While the current tests cover basic operations, consider adding tests for:
- Empty values:
params.append("key", "") - Special characters:
params.append("key+with+spaces", "value with spaces") - URL-unsafe characters:
params.append("key", "value#with#hash") - Unicode characters:
params.append("κλειδί", "τιμή") - Multiple operations: append + delete + sort
Let's check if there are any existing tests for these cases in other files:
#!/bin/bash
# Search for tests handling special characters or edge cases
rg -l "URLSearchParams.*(\"\"|[+#]|%20|\\u)" --type go11-110: 🛠️ Refactor suggestion
Add tests for missing URLSearchParams methods.
The test suite is missing coverage for several standard URLSearchParams methods:
delete()entries()forEach()keys()values()toString()(except in sort test)
Let's verify if these methods are implemented in the source:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
workflowBundler/workflowBundler.go (1)
35-35: Add documentation for the new Plugins field.Consider adding a comment to document the purpose and expected usage of the Plugins field in BundlerOptions.
+ // Plugins defines the list of esbuild plugins to be used during bundling Plugins []api.Plugin
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
workflowBundler/workflowBundler.go(2 hunks)
🔇 Additional comments (1)
workflowBundler/workflowBundler.go (1)
79-79: LGTM: Correct plugin configuration.
The plugins are properly passed from bundleOptions to the esbuild configuration.
Exported url search params to be usable outside
Added support for passing context for native APIs
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.