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.

Sign up for GitHub

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

Jump to bottom

feat: support for case-insensitive matches #168

Merged
merged 8 commits into from
Sep 22, 2022
Merged

feat: support for case-insensitive matches #168

merged 8 commits into from
Sep 22, 2022

Conversation

Sayan751
Copy link
Contributor

@Sayan751 Sayan751 commented Jul 13, 2022 •
edited by pr-preview bot
Loading

This commit updates the specification document with the support of
case-insensitive matches by introducing a third argument in the
URLPattern constructor. This additional document contains a single
boolean property named ignoreCase, which can be set to true to
enable case-insensitive matches. By default, this flag is set to false,
thus enabling case-sensitive matches.

Closes #148

Preview | Diff

@Sayan751
feat: support for case-insensitive matches
fe7fea5 
This commit updates the specification document with the support of
case-insensitive matches by introducing a third argument in the
`URLPattern` constructor. This additional document contains a single
boolean property named `ignoreCase`, which can be set to `true` to
enable case-insensitive matches. By default this flag is set to `false`,
thus enabling case-sensitive matches.
wanderview
wanderview requested changes Jul 13, 2022
View reviewed changes
Copy link
Member

@wanderview wanderview left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this! Overall it looks reasonable, but I have a few suggestions and requests. Also, I wouldn't mind having @domenic look at it for the object.assign usage.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@Sayan751
Copy link
Contributor Author

@wanderview @domenic Thanks for the review. I have made the changes, outlined. I think it should now be ready for the next round of review or merge.

Sayan751 added a commit to Sayan751/urlpattern-polyfill that referenced this pull request Jul 14, 2022
@Sayan751
refactor: ignoreCase
ffd3ed2 
More info: whatwg/urlpattern#168
wanderview
wanderview requested changes Jul 14, 2022
View reviewed changes
Copy link
Member

@wanderview wanderview left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again. I found a few more issues in the pass.

Note, there will probably be some delay in landing this PR even when its ready. I'm unsure about landing this without starting the process to change chromium. If you want to help there you could file a bug at crbug.com and include a link to to it in the commit message for this PR.

If you are interested in helping with changes to chromium I can support you with that. Otherwise I will need to carve out some time to do that in the next few weeks.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@Sayan751 Sayan751 requested a review from wanderview July 16, 2022 07:19
@Sayan751
Copy link
Contributor Author

@wanderview FYI created this to track the implementation task in Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=1345036

wanderview
wanderview approved these changes Jul 18, 2022
View reviewed changes
Copy link
Member

@wanderview wanderview left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks! Note I will probably wait to merge until implementation is close to landing.

@crowlKats crowlKats mentioned this pull request Jul 19, 2022
Merged
@wanderview wanderview mentioned this pull request Jul 29, 2022
Closed
wanderview
wanderview reviewed Aug 30, 2022
View reviewed changes
spec.bs
@@ -120,7 +120,7 @@ typedef (USVString or URLPatternInit) URLPatternInput;

[Exposed=(Window,Worker)]
interface URLPattern {
constructor(optional URLPatternInput input = {}, optional USVString baseURL);
constructor(optional URLPatternInput input = {}, optional USVString baseURL, optional URLPatternOptions options = {});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we also need a second constructor form to do what you want:

 constructor(optional URLPatternInput input = {}, optional URLPatternOptions options = {});

Otherwise you cannot specify options without also specifying a base URL second arg.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to fix this in a follow-up commit.

@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request Aug 31, 2022
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 31, 2022
@wanderview @chromium-wpt-export-bot
URLPattern: Add ignoresCase option.
b0ffaf7 
Implements spec changes from:

whatwg/urlpattern#168

Bug: 1345036
Change-Id: I659784cff5420603f8805aeed9bcd5852f97b5ae
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 31, 2022
@wanderview @chromium-wpt-export-bot
URLPattern: Add ignoresCase option.
04a8ae5 
Implements spec changes from:

whatwg/urlpattern#168

Bug: 1345036
Change-Id: I659784cff5420603f8805aeed9bcd5852f97b5ae
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 31, 2022
@wanderview
URLPattern: Add ignoresCase option.
de7fdfd 
Implements spec changes from:

whatwg/urlpattern#168

Bug: 1345036
Change-Id: I659784cff5420603f8805aeed9bcd5852f97b5ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3866651
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1041730}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 31, 2022
@wanderview @chromium-wpt-export-bot
URLPattern: Add ignoresCase option.
6644fd0 
Implements spec changes from:

whatwg/urlpattern#168

Bug: 1345036
Change-Id: I659784cff5420603f8805aeed9bcd5852f97b5ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3866651
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1041730}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 31, 2022
@wanderview @chromium-wpt-export-bot
URLPattern: Add ignoresCase option.
6d152e4 
Implements spec changes from:

whatwg/urlpattern#168

Bug: 1345036
Change-Id: I659784cff5420603f8805aeed9bcd5852f97b5ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3866651
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1041730}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 5, 2022
@wanderview @moz-wptsync-bot
Bug 1788644 [wpt PR 35729] - URLPattern: Add ignoresCase option., a=t…
682c587 
…estonly

Automatic update from web-platform-tests
URLPattern: Add ignoresCase option.

Implements spec changes from:

whatwg/urlpattern#168

Bug: 1345036
Change-Id: I659784cff5420603f8805aeed9bcd5852f97b5ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3866651
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1041730}

--

wpt-commits: 6d152e4018ca4f45ce93eea4985c516a518c3ad6
wpt-pr: 35729
wanderview and others added 3 commits September 22, 2022 16:28
@wanderview
Add second constructor signature.
9072872 
This is necessary to allow options to be provided without the second `baseURL` argument.
@wanderview
Fix secondary constructor.
826d077
@wanderview
Remove secondary constructor for now.
a53f5ca
@wanderview wanderview merged commit 7a09f4e into whatwg:main Sep 22, 2022
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
@wanderview @copybara-github
URLPattern: Add ignoresCase option.
14f1966 
Implements spec changes from:

whatwg/urlpattern#168

Bug: 1345036
Change-Id: I659784cff5420603f8805aeed9bcd5852f97b5ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3866651
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1041730}
NOKEYCHECK=True
GitOrigin-RevId: de7fdfdc52b4e01e709630eae6ebd29ad5524eed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wanderview wanderview wanderview approved these changes

@domenic domenic domenic left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No option to make URLPattern case insensitive
3 participants
@Sayan751 @domenic @wanderview