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

refactor(common-lib): use a more object-oriented-programming approach #326

Merged
merged 20 commits into from
Dec 18, 2024

Conversation

Tzal3x
Copy link
Collaborator

@Tzal3x Tzal3x commented Dec 12, 2024

Summary

I noticed that we need more flexibility as more features are coming in. Closes SEINT-428.

Moving on to an object-oriented approach by defining some classes and composing new ones based on them using the composition pattern will help us achieve that.

This way we can avoid spiraling down to multiple if-else clauses whenever a new feature request is coming.

In upcoming PRs, we can create a PageFetcherFactory class that generates a different pageFetcher which will contain different rpcURLs, domain resolution logic etc based on the type of site (based on the allowlist) that we will serve.

Changelog

  • Create a PageFetcher class.
  • Create a ResourceFetcher class.
  • Create a WalrusSitesRouter class.
  • rpcSelector is no longer a singleton. A differentRPCSelector can be used for: suin resolution, resource fetching and routes fetching without adding complex conditional flow control in the common/ library's code.
  • Fix environment variable parsing in the service worker.
  • Update tests to depict the new changes.

Copy link

vercel bot commented Dec 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
walrus-sites-sp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 18, 2024 9:24am
walrus-sites-sp-testnet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 18, 2024 9:24am
walrus-sites-sw ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 18, 2024 9:24am
walrus-sites-sw-testnet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 18, 2024 9:24am

Copy link

codspeed-hq bot commented Dec 12, 2024

CodSpeed Performance Report

Merging #326 will not alter performance

Comparing alextzimas/refactor-common-lib-move-to-ooo (46031da) with main (ab0f389)

Summary

✅ 3 untouched benchmarks

Copy link
Collaborator

@giac-mysten giac-mysten left a comment

Choose a reason for hiding this comment

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

Awesome! this is definitely in the right direction! Thank you @Tzal3x .
A few questions and suggestions on ways to improve further.

portal/common/lib/page_fetching.ts Outdated Show resolved Hide resolved
portal/common/lib/page_fetching.ts Outdated Show resolved Hide resolved
portal/common/lib/page_fetching.ts Outdated Show resolved Hide resolved
portal/server/app/route.ts Outdated Show resolved Hide resolved
portal/server/sentry.server.config.ts Outdated Show resolved Hide resolved
portal/worker/src/walrus-sites-sw.ts Outdated Show resolved Hide resolved
portal/common/lib/routing.ts Show resolved Hide resolved
portal/common/lib/resource.ts Outdated Show resolved Hide resolved
A utility class that makes it safer to load the environment
variables of the project.

By using this class, we can ensure that the environment
variables are loaded correctly and their data types are
correct.

---------

Co-authored-by: giac-mysten <[email protected]>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Update its' functions to depict the
new name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants