Add missing api::url::URLSearchParams in Body::Initializer.

[balusch]

Fix #3066

[balusch]

Hi @jasnell! Could you please review this PR?

I'm excited and eager to contribute to this project. While browsing the issues, I noticed that #3066 was proposed without a corresponding PR. I decided to address it.

I’m still trying to fully understand the issue (so the PR is still a WIP). I see that both EW_URL_ISOLATE_TYPES (from url.h) and EW_URL_STANDARD_ISOLATE_TYPES (from url-standard.h) are registered in the runtime. They seem similar, but url.h is not compliant with the WHATWG standard. Will they conflict, or does the latter override the former?

I suspect the problem arises because EW_URL_STANDARD_ISOLATE_TYPES overrides EW_URL_ISOLATE_TYPES, causing api::URLSearchParams to not match the JavaScript URLSearchParams object. As a result, the JavaScript URLSearchParams is coerced into a kj::String, leading to the content-type header error. I'm not very sure about that because the codebase is not easy to understand. If there is anything wrong, please correct it for me. :)

[balusch]

Hi, @jasnell. I’ve force-pushed an update to add the missing jsg::Ref<url::URLSearchParams> in Body::Initializer. I’ve tested both versions with different compatibilityDate, and they both worked well. I also added you as a co-author in the commit message and hope you don’t mind.

[jasnell]

@balusch ... thank you for the fix! Really appreciated!

[jasnell]

Looks like some linting issues with this. I'll fix those up and get this landed.

[jasnell]

hmm, actually that passed linting locally. Maybe a CI issue. Let me rerun CI.

[fhanau]

CI seems unwell, but looks more like for unrelated network/cache issues... hope those resolve again

[jasnell]

ah, ok, I see, there's an issue with our automatic type generation with two URLSearchParam definitions conflicting. We'll need to resolvre that before this can land.

@penalosa said that he'd take a look at what needs to be updated here to fix the type gen issue.

[fhanau]

Not to bikeshed some more, but I updated this to fix the comment line length (workerd uses KJ style and thus 100) and use links to the latest tagged release which are shorter.

[balusch]

Hi @fhanau Thanks for your update. However, the ci still failed and it seems that there is something wrong with the Bazel remote cache (maybe similar to this issue but not sure). Could you please re-trigger the ci?

[fhanau]

The Windows build appears thoroughly broken unfortunately – I'll try to see on Monday if flushing the cache entirely helps here.
Edit: Now it suddenly seems to be working 🤷

[fhanau]

@penalosa any remaining concerns or can I merge this?

[jasnell]

@fhanau ... let's do an internal CI run on this just to be extra safe.

[fhanau]

Regenerated the types as required by CI now, which revealed the next problem... URLSearchParams is listed twice in the types definition? @penalosa

[penalosa]

Regenerated the types as required by CI now, which revealed the next problem... URLSearchParams is listed twice in the types definition? @penalosa

I think that's fine. It's a bit messy but it shouldn't cause any type errors (since they point to the same definition). I can't really see any easy way around that unless we get a lot more invasive with the C++ definitions based on compat flag

[penalosa]

The Windows build appears thoroughly broken unfortunately – I'll try to see on Monday if flushing the cache entirely helps here. Edit: Now it suddenly seems to be working 🤷

This is a fork, so I don't think the cache is being used, which is probably why the builds are so slow