-
-
Notifications
You must be signed in to change notification settings - Fork 254
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
feat: add addCustomDomains for all platforms #321
Merged
FGRibreau
merged 16 commits into
FGRibreau:master
from
choznerol:feat/addCustomDomains-for-all-platforms
Nov 19, 2021
Merged
feat: add addCustomDomains for all platforms #321
FGRibreau
merged 16 commits into
FGRibreau:master
from
choznerol:feat/addCustomDomains-for-all-platforms
Nov 19, 2021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
choznerol
changed the title
Feat/add custom domains for all platforms
feat: add addCustomDomains for all platforms
Nov 2, 2021
choznerol
force-pushed
the
feat/addCustomDomains-for-all-platforms
branch
3 times, most recently
from
November 2, 2021 07:01
f0a374f
to
fa87cdf
Compare
choznerol
force-pushed
the
feat/addCustomDomains-for-all-platforms
branch
from
November 2, 2021 08:30
fa87cdf
to
8ef0850
Compare
👋 Regarding Rust, I would store the custom domains in another list and update the isinvalid method to check both list (BLACKLIST and custom_blocked_domains) |
choznerol
force-pushed
the
feat/addCustomDomains-for-all-platforms
branch
3 times, most recently
from
November 6, 2021 07:55
5c93cba
to
6342ec9
Compare
Co-authored-by: Buffele <Buffele@users.noreply.github.com>
Accroding to https://stackoverflow.com/a/27826181/6739302 : You can also use a RwLock instead of a Mutex to allow multiple concurrent readers. Currently, 'suffix_is_blacklisted()' (thus 'is_valid()') is limited to 1 concurrent reader due to the introductoin of Mutax in commit 'feat(rust): add add_custom_domains :'. This commit fixes this bottleneck.
This can be removed in Rust 1.30+ (https://stackoverflow.com/a/54954792/6739302). Accroding to package.json,Rust 1.38 is used.
This reverts commit cbdef35.
choznerol
force-pushed
the
feat/addCustomDomains-for-all-platforms
branch
from
November 9, 2021 01:24
a751e8e
to
8d6abd3
Compare
Thanks! Released in v4.1.0 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow the user to add more domains to block.
Inspired by #302, but for all 9 platforms.
TODOs
Questoins
[Solved]
1. In some platforms, domain list is not a mutable global variableIn some platforms, the domain list is maintained as a static property evaluated at compile-time, so the mutate global variable strategy used in ruby/python/js/php probably won't work easily.RustUpdate: I'll try 2.
static BLACKLIST: &'static [&'static str] = &[{{ &listSTR }}]
looks static. I'm not very familiar with Rust and can only think of the below 2 strategies:Refactor
BLACKLIST
into something mutable, like thevar ( blacklist )
in go.Add another mutable global variable
custom_domains
to augmentBLACKLIST
. Probably have some performance benefit over 1. ? (not sure)Would love to know Which one is preferred, or perhaps there is another better strategy?
Elixir (and probable Clojure too)Update: I realized this can be achieved with a simple Agent. WIP.
Module Attributes like
@blacklist
are inlined at compile time therefore immutable (ref).From the point of view of FP, what we do in ruby/python/js/php is not pure and has global side effects:
I double if it is desired (or even technically feasible) to have a similar API in Elixir/Clojure. Would love to know what the desired design for Elixir/Clojure may look like.