kenchris / urlpattern-polyfill

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

feat: case-insensitive match for pathname #100

Closed Sayan751 closed 2 years ago

Sayan751 commented 2 years ago

Related to https://github.com/WICG/urlpattern/issues/148. This PR adds support for case-insensitive match for pathname. It adds a caseSensitivePath flag to the init object.

Example:

import { strictEqual } from 'assert';

const pattern = new URLPattern({ pathname: '/home', caseSensitivePath: false });

const url = new URL('https://example.com/home');
strictEqual(pattern.test(url.href), true, url.href);

url.pathname = '/HOME';
strictEqual(pattern.test(url.href), true, url.href);
kenchris commented 2 years ago

We are matching the spec so maybe file an issue on the spec as well?

Sayan751 commented 2 years ago

We are matching the spec so maybe file an issue on the spec as well?

Thanks! I am continuing the discussion already in the linked issue.

wanderview commented 2 years ago

Note, I think we should land WPT changes upstream in chromium or wpt before bringing them here. Chromium will auto-sync to wpt and other browsers, but we don't have that setup for the polyfill repo.

And thanks to @Sayan751 for working on this!

kenchris commented 2 years ago

This looks good to me, have the upstream WPT changed landed @wanderview ?

I am fine with landing this and making a new release.

kenchris commented 2 years ago

ping @wanderview

wanderview commented 2 years ago

No they have not. Sorry, for the delay.

kenchris commented 2 years ago

@wanderview so you want me to wait with landing this until that has landed right?

wanderview commented 2 years ago

I think that would be best. I'll try to get to the upstream work this week.

wanderview commented 2 years ago

Note, I think I'm going to simplify the test changes in my upstream CL. I don't think we need this many test cases for ignoreCase.

wanderview commented 2 years ago

The spec changes have landed and I have sent the intent to ship in chrome M107 here:

https://groups.google.com/a/chromium.org/g/blink-dev/c/KgAdo3kB1wc/m/UN70zkeNAwAJ

Note, I recommend using the WPT tests from here instead of the WPT tests currently in this PR:

https://github.com/web-platform-tests/wpt/commit/6d152e4018ca4f45ce93eea4985c516a518c3ad6

Unfortunately I'm not sure how to make this big of a change on someone else's PR.

kenchris commented 2 years ago

Hi @Sayan751 do you want to update your PR? Or do you want us to redo it and give you credit?

Sayan751 commented 2 years ago

@kenchris I am sorry for this, but I have no time to update the PR. Please feel free to make the changes as you see fit.

wanderview commented 2 years ago

We could just land this and then do a fast follow-up to sync the upstream WPT json file.

kenchris commented 2 years ago

Sure. Let's do that.

kenchris commented 2 years ago

I will file an issue to track it

kenchris commented 2 years ago

OK filed https://github.com/kenchris/urlpattern-polyfill/issues/106