-
Notifications
You must be signed in to change notification settings - Fork 144
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
Selective blocking #1512
Selective blocking #1512
Conversation
8493a14
to
db07447
Compare
db07447
to
10d8a9d
Compare
e6bd589
to
d88f85e
Compare
1f11093
to
f748fe5
Compare
873e2d3
to
210ae12
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing!
I'm all for merging it in this state as it is already too big to handle.
Few things will require to be addressed before it can be released, for example Safari support.
beforeEach(() => { | ||
exception = { | ||
blocked: false, | ||
blockedDomains: ['foo.org'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a weird setup, having a domain trusted and blocked at the same time. Is such a case possible to reach in production?
Also, initializing it with each tests, might be easier to read, since it makes the tests stand-alone and independent of each other. If code sharing is the concern, you can use a helper function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is expected to have both blockedDomains
and allowedDomains
at the same time. In this way we remember the opposite exceptions when users toggle the blocking mode.
WIP.
Usage of the
TrackerException
model in the background: