Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(URLPattern): Handle search parameters correctly in URLPattern with base #49

Conversation

yazan-abdalrahman
Copy link

…parameters when a base URL is provided. This fixes the discrepancy where URLPattern in Deno was returning null for URLs with search parameters, unlike the behavior observed in Chrome.

Adjusted URLPattern parsing to include search parameters when matching against base URLs. Fixes #denoland/deno#24266

…parameters when a base URL is provided. This fixes the discrepancy where URLPattern in Deno was returning null for URLs with search parameters, unlike the behavior observed in Chrome.

Adjusted URLPattern parsing to include search parameters when matching against base URLs.
Fixes #denoland/deno#24266
@CLAassistant
Copy link

CLAassistant commented Jul 22, 2024

CLA assistant check
All committers have signed the CLA.

@yazan-abdalrahman
Copy link
Author

@crowlKats
Hello,

As I previously mentioned, I found that when the "URLPattern" is constructed in Deno, it attempts to establish the pattern; however, if no pattern is specified, the default pattern for the "search field" is set to None("")

so in 'p.exec' , it attempts to extract patterns and compares them with those generated by the constructor.
so if there is no match, it will return NULL

check out this example, which attempts to match ?id=2 with ?id=3 ,the result no match, null was returned.
image

and check out this example, it tries to match ?id=3 with ?id=3  ,the result matched, The object was returned
image

So, if no pattern was specified, the search pattern will be set to NONE, meaning there would be no match and no result.
image

So I wrote change to set the search pattern default value to .
'
' indicates that it will fit any pattern.
image


I have an additional fix. I like it better; it's more like Chrome behavior, but I'll have to rewrite every test in the "url-pattern" from start if you want this change. It will take me a few days to complete the rewrites.

image
image
image

so if we apply this change, we can remove our construct code of UrlPattern Init ,this is simpler and has fewer difficult code snippets.
but as it changes the default value of pattern for every field, its need to  rewrite  tests from  scratch.

@yazan-abdalrahman
Copy link
Author

@crowlKats
Please see the lint. It failed in line I didn't write it.

@crowlKats
Copy link
Member

crowlKats commented Jul 22, 2024

@yazan-abdalrahman the tests in the JSON file come from WPT, which are not to be changed since they match the standard specification.
I'll tkae a deeper look at this PR later today or tomorrow.

@yazan-abdalrahman
Copy link
Author

@crowlKats
Hello ,it would be better to reimplement the "URL Pattern" constructor to be depends on the URL. With regard to the update test in "wtp," I don't think its tandard specification. i think it should be changed because the result depends on the generated pattern when creating the "URL Pattern," so changing the default pattern will need to change the test.

#50
However, this is a workaround for the issue and no tests changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants