Add missing api::url::URLSearchParams in Body::Initializer.#3151
Add missing api::url::URLSearchParams in Body::Initializer.#3151jasnell merged 4 commits intocloudflare:mainfrom
Conversation
|
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 I suspect the problem arises because |
c8ba969 to
1650021
Compare
|
Hi, @jasnell. I’ve force-pushed an update to add the missing |
|
@balusch ... thank you for the fix! Really appreciated! |
|
Looks like some linting issues with this. I'll fix those up and get this landed. |
|
hmm, actually that passed linting locally. Maybe a CI issue. Let me rerun CI. |
|
CI seems unwell, but looks more like for unrelated network/cache issues... hope those resolve again |
|
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. |
c0f5da8 to
2f58c06
Compare
|
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. |
|
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? |
|
The Windows build appears thoroughly broken unfortunately – I'll try to see on Monday if flushing the cache entirely helps here. |
|
@penalosa any remaining concerns or can I merge this? |
|
@fhanau ... let's do an internal CI run on this just to be extra safe. |
2f58c06 to
67c375d
Compare
Co-authored-by: James M Snell <jsnell@cloudflare.com> Signed-off-by: Jianyong Chen <baluschch@gmail.com>
67c375d to
acefd77
Compare
|
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 |
This is a fork, so I don't think the cache is being used, which is probably why the builds are so slow |
Fix #3066