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.

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

Conversation

choznerol
Copy link
Contributor

@choznerol choznerol commented Nov 2, 2021

Allow the user to add more domains to block.

Inspired by #302, but for all 9 platforms.

TODOs

  • clojure (Tests need cbdef35 to run)
  • elixir
  • go
  • javascript
  • node
  • php
  • python
  • ruby
  • rust

Questoins

[Solved] 1. In some platforms, domain list is not a mutable global variable

In 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.

Rust

Update: 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:

  1. Refactor BLACKLIST into something mutable, like the var ( blacklist ) in go.

  2. Add another mutable global variable custom_domains to augment BLACKLIST. 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:

// test/platform.node.test.js

  isValid('foo@youtube.com')
  isValid('foo@google.com')
  isValid('ok@gmail.com')

  MailChecker.addCustomDomains(['youtube.com', 'google.com'])  // Global side effect 😨

  isInvalid('foo@youtube.com')  // Same input, different result. Not pure 😨
  isInvalid('foo@google.com')
  isValid('ok@gmail.com')

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.

@choznerol choznerol changed the title Feat/add custom domains for all platforms feat: add addCustomDomains for all platforms Nov 2, 2021
@choznerol choznerol force-pushed the feat/addCustomDomains-for-all-platforms branch 3 times, most recently from f0a374f to fa87cdf Compare November 2, 2021 07:01
@choznerol choznerol marked this pull request as ready for review November 2, 2021 07:03
@choznerol choznerol force-pushed the feat/addCustomDomains-for-all-platforms branch from fa87cdf to 8ef0850 Compare November 2, 2021 08:30
@choznerol choznerol marked this pull request as draft November 2, 2021 16:21
@FGRibreau
Copy link
Owner

👋 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 choznerol force-pushed the feat/addCustomDomains-for-all-platforms branch 3 times, most recently from 5c93cba to 6342ec9 Compare November 6, 2021 07:55
@choznerol choznerol marked this pull request as ready for review November 6, 2021 14:56
choznerol and others added 16 commits November 9, 2021 09:18
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.
@choznerol choznerol force-pushed the feat/addCustomDomains-for-all-platforms branch from a751e8e to 8d6abd3 Compare November 9, 2021 01:24
@FGRibreau FGRibreau merged commit 3b36a11 into FGRibreau:master Nov 19, 2021
@FGRibreau
Copy link
Owner

Thanks! Released in v4.1.0

@choznerol choznerol deleted the feat/addCustomDomains-for-all-platforms branch November 20, 2021 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants