URLSearchParams Implementation #54

etiennemartin · 2023-06-06T16:23:21Z

This PR is a proposal for the implementation of the URLSerachParams class provided by NodeJS. You can find the documentation here: Docs.

This implementation is continuation of the work done to implement URL itself. It tries to implement as much of the NodeJS functionality as possible for goja.

The following is currently unsupported in the implementation:

ctor(iterable): Using function generators
sort(): Since the backing object is a url.URL which backs the data as a Map, we can't reliably sort the entries
[] operator: Not sure if we are able to override this with goja.

If the general approach is something we are happy with, I will look at making sorting and ordering something built in. This would require more work since we wouldn't be able to rely on the backing object being a url.URL struct. Right now we use the Encode() of url.URL method in several places.

etiennemartin · 2023-06-06T16:24:17Z

url/url.go

+	})
+
+	// search Params
+	defineURLAccessorProp(r, p, "searchParams", func(u *url.URL) interface{} {


Most of the code in this file was simply moved from module.go. I've only added this new method as part of the URL implementation (Along with tests).

etiennemartin · 2023-06-06T23:28:47Z

Hmmm. Looks like I missed something in the push. I'll update this PR tonight.

etiennemartin · 2023-06-07T23:15:29Z

The latest changes shows that a random change can affect the ordering of the url parameters. This results in flaky tests. I'll have to find a way to get this working with something consistent. My idea right now is to wrap a struct and maintain a separate list of ordered parameters.

dop251 · 2023-06-08T09:39:30Z

Hi. I've just realised that in node the url implementation is actually written in javascript: https://github.com/nodejs/node/blob/main/lib/internal/url.js

I think for maximum compatibility the implementation should be derived from there, converted to Go of course.

I had a quick look at the code, one thing that strikes me is that they set the search property of the associated URL every time the query parameters are updated... Not quite sure why not make it a lazy update since URL.search is an accessor property.

As for the iteration, I guess goja needs to expose something that would allow doing for ... of from Go.

Not quite sure what you mean by [] operator.

etiennemartin · 2023-06-08T12:22:20Z

I think for maximum compatibility the implementation should be derived from there, converted to Go of course.

How important is the idea of maximum compatibility here? I'm just wondering since this would invalidate the implementation of URL and URLSearchParams. If we aim to be as close as possible it would suggest a re-write for the most part. My assumption here was to ensure that the implementation would behave the same, not exactly implemented the same. Just trying to align expectations moving forward.

I had a quick look at the code, one thing that strikes me is that they set the search property of the associated URL every time the query parameters are updated... Not quite sure why not make it a lazy update since URL.search is an accessor property.

That's interesting, I see what you're saying. I wonder if this is just an oversight, or lack of need to optimize? Not sure.

Not quite sure what you mean by [] operator.

When I was looking to write the implementation, I got tripped up on the use of iterators. I managed to get most examples in the docs to work using normal collections (Array, Map). For the most part it should be fine. It's just a bit odd since the docs reference ES6 and goja supports 5.1.

I guess overall the big question for me is how closely do we want the API's implementation to be similar to NodeJS's implementation. I was about to change the backing object to URL and URLSearchParams to adhere to a strict ordering of query params.

dop251 · 2023-06-08T17:54:29Z

It should be as close as practically achievable. As it's not that much code and there is a reference implementation in javascript, it shouldn't be too difficult to implement it "as-is". See my comment here: #40 (comment), I think you should follow this route, but instead of using url.Values you should have a different implementation which is a ported version from the reference code.

etiennemartin · 2023-06-08T18:57:35Z

See my comment here: #40 (comment), I think you should follow this route

I have an implementation in the works right now with something like this. I'll post an update to this PR once I've done the work.

etiennemartin · 2023-06-12T16:40:11Z

I've pushed a pretty big change that changes the backing structure. This brings us closer to the native implementation and allows us to maintain the order of the query parameters. I just need to look into some encoding oddities in the URL class right now (See commented out tests).

dop251 · 2023-06-12T20:54:50Z

What's the reason for using own structure instead of url.URL? I thought it worked quite well...

etiennemartin · 2023-06-13T12:41:30Z

What's the reason for using own structure instead of url.URL? I thought it worked quite well...

I opted to move away from url.URL because of the url search parameters. The search parameters are stored as an unordered map. This makes it impossible to maintain the order of the search queries and makes writing non-flaky tests nearly impossible. So I opted in maintaining manually parsing them and maintaining an array. I was hoping the language would change this but seeing this:

golang/go#29985

I don't have much hope in this change making it in for a while.

That being said, there's an issue I'm running into now with goja. The search parameter's constructor is expected to take in an object into the constructor. I need to take another look, but the object properties seem to be non-deterministic in their order when they are passed from Javascript -> Go. This might make writing tests for this new class really hard.

dop251 · 2023-06-13T13:56:19Z

Just don't use URL.Query(). I mean it would make sense if you abandoned net.URL completely, but you seem to be using the parser and then just copying everything into a custom struct which does not make a lot of sense in my opinion.

etiennemartin · 2023-06-13T19:56:19Z

I'll make the change back. I think you're right. I'll change the nodeURL struct to be a url.URL along with an array of searchParams. The original idea was to move closer to the implementation that NodeJS had.

url/urlsearchparams.go

url/nodeurl.go

etiennemartin · 2023-07-04T14:02:31Z

I've updated the code and removed the use of Export() with each execution. This approach also makes use for the ForOf implementation you provided. Sorry for the delay I was away for a bit. Let me know what you think.

dop251

Hi. Thanks for continuing working on this and apologies for the delay. I've added some comments, please have a look.

url/nodeurl.go

dop251 · 2023-07-27T12:32:29Z

url/nodeurl.go

+	str := ""
+	sep := ""
+	for _, v := range s {
+		str = fmt.Sprintf("%s%s%s", str, sep, v.Encode())


Not the most efficient way. Even a simple string concatenation would be better I think. But best to use strings.Builder.

url/nodeurl.go

dop251 · 2023-07-27T12:56:08Z

url/urlsearchparams.go

+		name := call.Arguments[0].String()
+		if len(call.Arguments) > 1 {
+			value := call.Arguments[1].String()
+			arr := searchParams{}


Perhaps a better approach would be finding an element and doing an inline copy, rather than always allocate and copy.

I've since simplified this approach, but the reason I used approach was to allow for deletion of multiple items in one pass. The Search Params might have have many entries with the name. Which would result in all of them being removed.

It still doesn't justify the allocation and the full copy. Even if multiple elements are deleted it's still possible to do it with in-place replacement and with a single pass.

url/urlsearchparams.go

url/nodeurl.go

dop251

It's getting there, just a few minor things that need to be addressed.

dop251 · 2023-08-29T15:32:43Z

url/urlsearchparams.go

+	query := searchParams{}
+
+	o := v.ToObject(r)
+	ex := r.Try(func() {


No need to use Try here as this function is only called by the Runtime. If something within ForOf throws an exception the resulting panic will be handled by the caller of this function.

dop251 · 2023-08-29T15:33:16Z

url/urlsearchparams.go

+func buildParamsFromMap(r *goja.Runtime, v goja.Value) *url.URL {
+	query := searchParams{}
+	o := v.ToObject(r)
+	ex := r.Try(func() {


dop251 · 2023-08-29T15:35:13Z

url/nodeurl.go

+}
+
+func (nu *nodeURL) getValues(name string) []string {
+	vals := []string{}


This is generally frowned upon. Unless you absolutely have to have the returned value to be non-nil, it's better to just do var vals []string

Interesting, the reason I had done this was to avoid a crash when we called len(vals) where the value is returned. I didn't know that len(x) could take nil as a value. Good catch!

dop251 · 2023-08-29T15:37:43Z

url/urlsearchparams.go

+			case reflectTypeString:
+				var str string
+				r.ExportTo(v, &str)
+				u = buildParamsFromString(str)


It's easier to do u = buildParamsFromString(v.String())

dop251 · 2023-08-29T15:39:28Z

url/nodeurl.go

+	} else {
+		vals = append(vals, fmt.Sprintf("%s=%s", sp.name, sp.value))
+	}
+	return strings.Join(vals, "&")


Why do you need the slice and the Join() here?

We don't need it at all, this was a relic from a previous implementation. I'll simplify this.

dop251 · 2023-08-29T15:40:02Z

url/urlsearchparams.go

+}
+
+// Currently not supporting the following:
+//   - ctor(iterable): Using function generators


Is this still relevant?

With the current implementation this is still relevant. I don't have any code to handle the use of generator functions. It's treated as an Object that generates an empty object. I would need to find a way to call the generator function and process it's output. Any leads on how I could do that given a goja.Object that represents a generator function?

The output of a generator function is an iterable for which you can use ForOf. BTW, an array is also an iterable, so you can generalise it for objects:

func buildParamsFromObject(r *goja.Runtime, v goja.Value) *url.URL { o := v.ToObject(r) if o.GetSymbol(goja.SymIterator) != nil { return buildParamsFromIterable(r, o) } ... }

buildParamsFromIterable is pretty much the same as the current buildParamsFromArray.

Awesome! I totally missed the goja.GetSymbol() functionality in goja. This is great! I'll make this change and push it up. Thanks for the heads up.

dop251 · 2023-08-29T15:44:58Z

url/urlsearchparams.go

+		name := call.Arguments[0].String()
+		if len(call.Arguments) > 1 {
+			value := call.Arguments[1].String()
+			arr := searchParams{}


It still doesn't justify the allocation and the full copy. Even if multiple elements are deleted it's still possible to do it with in-place replacement and with a single pass.

…hing

etiennemartin · 2023-08-31T19:20:18Z

I've updated the code base on your comment, the only one I couldn't answer is the one related to the generator functions. I'm not quite sure how I can run a generator function from Go in a clean manner. I'm looking for a way to detect if the parameter is a generator function or not. Is there support for this using goja? If so can you help by pointing me in the right direction?

I've tried the following to see if I can call the generator function and the assert call seems to fail:

callable, ok := goja.AssertFunction(v)
v. if ok {
  ret, err := callable(nil)
  
  // ...	
}

In the example above, v is a goja.Object. But when I dig into it, it's data property seems to be a goja.generatorObject which I'm not sure how to call.

dop251 · 2023-09-02T10:14:41Z

url/urlsearchparams.go

+}
+
+// Currently not supporting the following:
+//   - ctor(iterable): Using function generators


The output of a generator function is an iterable for which you can use ForOf. BTW, an array is also an iterable, so you can generalise it for objects:

func buildParamsFromObject(r *goja.Runtime, v goja.Value) *url.URL { o := v.ToObject(r) if o.GetSymbol(goja.SymIterator) != nil { return buildParamsFromIterable(r, o) } ... }

buildParamsFromIterable is pretty much the same as the current buildParamsFromArray.

dop251 · 2023-09-02T10:16:55Z

url/urlsearchparams.go

+	return u
+}
+
+func stringFromValue(r *goja.Runtime, v goja.Value) string {


Why do you need this? The standard .String() should handle all these cases.

Sounds good, I'll remove this.

etiennemartin added 2 commits June 6, 2023 12:14

Bump to latest goja version

a61bb24

Implement URLSearchParams class along side URL

ee4dffc

etiennemartin commented Jun 6, 2023

View reviewed changes

etiennemartin added 2 commits June 6, 2023 21:32

Remove Has() method declared in later golang version

d89596d

Converted type reflection to match codebase

ddd22e9

Refactored module to use nodeURL instead of native url.URL

9ffbbb6

More stable tests

42e1c7a

etiennemartin added 2 commits June 13, 2023 16:22

Remove use of custom struct for nodeURL

22f157c

Comment out pre-failing tests from before

685aa3a

dop251 reviewed Jun 14, 2023

View reviewed changes

url/urlsearchparams.go Outdated Show resolved Hide resolved

url/urlsearchparams.go Outdated Show resolved Hide resolved

url/urlsearchparams.go Outdated Show resolved Hide resolved

url/urlsearchparams.go Outdated Show resolved Hide resolved

url/nodeurl.go Outdated Show resolved Hide resolved

Removed the use of Export()

f27e615

Lazy load Search Parameters on URL

36b813c

dop251 requested changes Jul 27, 2023

View reviewed changes

etiennemartin added 5 commits July 30, 2023 22:10

Simplified and cleaned up some of the methods

ec4daaa

Removed the use of a String array from the search param struct

cc55957

Re-enable tests

8c4d82b

Fixed parsing issue where it would excessivily split.

8b99b02

Removed several uses of direct access into the call.Arguments array

403311a

Added a few extra tests

b3b7721

etiennemartin requested a review from dop251 August 29, 2023 14:44

dop251 requested changes Aug 29, 2023

View reviewed changes

etiennemartin added 2 commits August 31, 2023 09:22

Removed use of Try and simplified methods that did extra work for not…

0364a97

…hing

Inline deletion of parameters

31e0720

etiennemartin requested a review from dop251 August 31, 2023 19:19

dop251 requested changes Sep 2, 2023

View reviewed changes

etiennemartin added 2 commits September 7, 2023 15:49

Remove the use of stringFromValue for String()

1ddbb4f

Add support for Function Generators

c6c0ef7

etiennemartin requested a review from dop251 September 7, 2023 19:51

dop251 changed the base branch from master to urlsearchparams September 12, 2023 13:46

dop251 merged commit 76fdc05 into dop251:urlsearchparams Sep 12, 2023
8 checks passed

URLSearchParams Implementation #54

URLSearchParams Implementation #54

Conversation

etiennemartin commented Jun 6, 2023

Choose a reason for hiding this comment

etiennemartin commented Jun 6, 2023

etiennemartin commented Jun 7, 2023

dop251 commented Jun 8, 2023

etiennemartin commented Jun 8, 2023

dop251 commented Jun 8, 2023

etiennemartin commented Jun 8, 2023

etiennemartin commented Jun 12, 2023

dop251 commented Jun 12, 2023

etiennemartin commented Jun 13, 2023

dop251 commented Jun 13, 2023

etiennemartin commented Jun 13, 2023

etiennemartin commented Jul 4, 2023

dop251 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dop251 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etiennemartin commented Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etiennemartin commented Aug 31, 2023 •

edited

Loading