URLSearchParams Implementation

[etiennemartin]

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]

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

[etiennemartin]

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]

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]

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]

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: https://github.com/dop251/goja_nodejs/pull/40#issuecomment-1337135457, 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]

See my comment here: https://github.com/dop251/goja_nodejs/pull/40#issuecomment-1337135457, 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]

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]

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

[etiennemartin]

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:

https://github.com/golang/go/issues/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]

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]

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.

[etiennemartin]

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.

[etiennemartin]

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.