kenchris / urlpattern-polyfill

URLPattern polyfill
https://www.npmjs.com/package/urlpattern-polyfill
MIT License
268 stars 31 forks source link

polyfill inconsistent with chrome by how it handle search #90

Closed jimmywarting closed 2 years ago

jimmywarting commented 2 years ago

this example works in chrome & Deno

new URLPattern({ search: "a=:a" }).exec('http://localhost?a=x/y')

but that does not work with the polyfill

the problem seem to be in where the search param value includes a /

replace x/y with just xy and it works

kenchris commented 2 years ago

It is probably lacking a WPT (Web Platform Test) for that case.

@wanderview

wanderview commented 2 years ago

I took a glance through the code and nothing is jumping out at me. It will likely need some active debugging.

wanderview commented 2 years ago

I think I see the problem. We try to tell path-to-regexp not to use a "delimiter" for non-pathname components like search. To do this we pass an empty string delimiter in the options dictionary when compiling the pattern. Unfortunately it looks like path-to-regexp interprets empty string as missing and applies its default delimiters instead:

https://github.com/kenchris/urlpattern-polyfill/blob/main/src/path-to-regex-modified.ts#L197

Since '/' is one of the default path-to-regexp delimiters it breaks the scenario reported here.

kenchris commented 2 years ago

We could fix that in our local copy. I think that would be fine

SanderElias commented 2 years ago

@wanderview, I added a test for this to the suite and updated the code to use ?? instead of || in the relevant parts. (see #95 for details)

@jimmywarting Can you give version 5.0.1 a try and see if it works for you too?

I will close this issue now, as it should be fixed. Depending on feedback, we might need to reopen.