Closed unrenamed closed 1 month ago
@unrenamed fantastic suggestion! Feel free to PR, will get this merged 🙏
@unrenamed added this to oss.gg btw, feel free to claim this issue and make the PR (instructions here)
I made a PR for it https://github.com/dubinc/dub/pull/1311
I guess the early bird really does get the worm... 😅
😅
Since this was @unrenamed's idea, I've closed @sudipb7's PR to give a chance for @unrenamed to make the changes instead.
@unrenamed do you want to comment /assign
and get the issue assigned to you by OSS.gg, and create the PR instead?
Well, since you've already closed @sudipb7's PR, I guess I can only thank you and create the new one including @devkiran's review comments. Will do in a moment ✍️
/assign
Assigned to @unrenamed! Please open a draft PR linking this issue within 48h ⚠️ If we can't detect a PR from you linking this issue in 48h, you'll be unassigned automatically 🕹️ Excited to have you ship this 🚀
Understood, no problem at all! Thanks for letting me know.
Currently, the validation logic for checking if an email belongs to a disposable domain uses
SMEMBERS
to retrieve all the domains from the Redis setdisposableEmailDomains
, followed by anincludes()
check in the application code. This approach can be inefficient, especially as the size of the set grows, since both the RedisSMEMBERS
command and the subsequent membership check have an overall time complexity of O(N).https://github.com/dubinc/dub/blob/628eb46bfe7012ac3f7d316e0584265b2d999c00/apps/web/lib/actions/create-user-account.ts#L33-L42
Suggested Improvement: To optimize this, you can directly use Redis’s
SISMEMBER
command, which has a constant time complexity O(1), to check if the domain is part of the set without retrieving all the elements.Proposed Code Change:
Another benefit the app can get from the proposed solution is lower network overhead: you will avoid fetching the entire set from Redis, reducing the amount of data transferred between Redis and the application.
Links: