Skip to content

feat: add excludeUrls and modify urls in WebRequestFilter for better URL filtering #44692

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

Merged
merged 16 commits into from
Feb 17, 2025

Conversation

alicelovescake
Copy link
Member

@alicelovescake alicelovescake commented Nov 18, 2024

Description of Change

This PR introduces the following properties to the WebRequestFilter object:

  • excludeUrls property. Specify URL patterns that should be excluded from matching.
  • add deprecation message to existing urls property if it contains an empty array.

Why add excludeUrls?

Previously, the WebRequestFilter object only supported the urls property, which allowed developers to specify patterns for URLs that should be matched. However, there was no straightforward way to exclude certain URLs from being matched. This limitation made it challenging to implement filtering logic where certain URLs needed to be explicitly excluded.

Why add deprecation message to urls property with empty array?

Previously, an empty urls array meant all URLs were included. This doesn't make sense. If the developer wants all urls included, they should use all_urls, which is a special URL pattern that matches everything.

Changes

New Property: excludeUrls (optional)

Developers can now define urls and excludeUrls in their WebRequestFilter objects. The filtering logic will first check if a request matches any pattern in excludeUrls. If it does, the request will be excluded. If not, the request will be checked against the urls patterns.

const filter = {
 urls: ['https://example.com/*'],
 excludeUrls: ['https://example.com/exclude/*']
};

session.webRequest.onBeforeRequest(filter, (details, callback) => {
  // Handle the request with all urls from example.com except ones that end with exclude
});

const filter2 = {
  urls: ['<all_urls>'],
  excludeUrls: ['https://example.com/*']
};

Tests

Deprecation warning
image

Checklist

Release Notes

Notes: Added excludeUrls to webRequest filter and deprecated the use of empty arrays in urls property.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Nov 18, 2024
@alicelovescake alicelovescake marked this pull request as draft November 18, 2024 02:27
@alicelovescake alicelovescake changed the title feat: add excludeUrls to WebRequestFilter for better URL filtering feat: add excludeUrls, add includeUrls, deprecate urls to WebRequestFilter for better URL filtering Nov 19, 2024
@alicelovescake alicelovescake marked this pull request as ready for review November 20, 2024 21:41
@alicelovescake
Copy link
Member Author

@miniak Tagging you in this PR because you recently added types to the filter! Open to any feedback! 🙇‍♀️

@alicelovescake alicelovescake self-assigned this Nov 21, 2024
@samuelmaddock samuelmaddock added the semver/minor backwards-compatible functionality label Nov 21, 2024
Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

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

API LGTM

The rationale seems very clear to me and I think having an <all-urls> special value that aligns with the web standard is a cleaner way to have a catch-all value than the empty array approach from the previous API.

Copy link
Member

@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

excludeUrls looks good 👍 I have thoughts about following established standards which I left feedback for.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Nov 27, 2024
@alicelovescake alicelovescake changed the title feat: add excludeUrls, add includeUrls, deprecate urls to WebRequestFilter for better URL filtering feat: add excludeUrls and modify urls in WebRequestFilter for better URL filtering Dec 1, 2024
@itsananderson
Copy link
Member

Small nit: The PR description uses <all-urls> but the PR body uses <all_urls>.

The deprecation warning seems like something worth calling out in the breaking changes doc. I think maybe as a "Default Changed" breaking change?

Copy link
Member

@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

API LGTM

@alicelovescake alicelovescake requested a review from a team as a code owner February 6, 2025 19:27
@samuelmaddock samuelmaddock added the target/35-x-y PR should also be added to the "35-x-y" branch. label Feb 12, 2025
@VerteDinde VerteDinde merged commit 02be7c1 into main Feb 17, 2025
59 checks passed
@VerteDinde VerteDinde deleted the alice-add-exclude-url branch February 17, 2025 20:40
@release-clerk
Copy link

release-clerk bot commented Feb 17, 2025

Release Notes Persisted

Added excludeUrls to webRequest filter and deprecated the use of empty arrays in urls property.

@trop
Copy link
Contributor

trop bot commented Feb 17, 2025

I was unable to backport this PR to "35-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/35-x-y and removed target/35-x-y PR should also be added to the "35-x-y" branch. labels Feb 17, 2025
alicelovescake added a commit that referenced this pull request Feb 18, 2025
…URL filtering (#44692)

* feat: add excludeUrls to web request filter

* refactor: add deprecated field

* test: update tests

* lint: newline

* docs: improve API doc

* fix: add is filter defined property to match all urls

* refactor: remove includeUrls

* refactor: remove typescript binding

* refactor: all_url

* refactor: remove isDefined methods

* refactor: remove comment

* fix: logic

* docs: add to breaking changes
@trop
Copy link
Contributor

trop bot commented Feb 19, 2025

@alicelovescake has manually backported this PR to "35-x-y", please check out #45678

jkleinsc pushed a commit that referenced this pull request Feb 20, 2025
…URL filtering (#45678)

* feat: add excludeUrls and modify urls in WebRequestFilter for better URL filtering (#44692)

* feat: add excludeUrls to web request filter

* refactor: add deprecated field

* test: update tests

* lint: newline

* docs: improve API doc

* fix: add is filter defined property to match all urls

* refactor: remove includeUrls

* refactor: remove typescript binding

* refactor: all_url

* refactor: remove isDefined methods

* refactor: remove comment

* fix: logic

* docs: add to breaking changes

* docs: remove unneeded section
@trop trop bot added merged/35-x-y PR was merged to the "35-x-y" branch. and removed in-flight/35-x-y labels Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review/approved ✅ merged/35-x-y PR was merged to the "35-x-y" branch. semver/minor backwards-compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants