-
Notifications
You must be signed in to change notification settings - Fork 65
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
Add subdomain support #52
Conversation
Enables the sandbox feature to work when the node is hosted on a subdomain. Added to allow hosting on exiting domains.
Enables the arns feature to work when the node is hosted on a subdomain. Added to allow hosting on exiting domains.
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.
Thanks for the PR! Just one request re reducing duplication, otherwise this looks great.
src/middleware/arns.ts
Outdated
nameResolver: NameResolver; | ||
}): Handler => | ||
asyncMiddleware(async (req, res, next) => { | ||
const rootHostSubdomainLength = rootHost.split('.').length - 2; |
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.
@kay-is, let's reduce duplication by calculating this value in config.ts
(create a ROOT_HOST_SUBDOMAIN_LENGTH
const) and then passing that into the middlewares. Alternately, you could create a utility function and use it in both locations, but I think the config constant approach is probably simpler for now.
src/middleware/sandbox.ts
Outdated
if (req.subdomains.length === 1) { | ||
return req.subdomains[0]; | ||
function getRequestSandbox(req: Request, rootHost: string): string | undefined { | ||
const rootHostSubdomainLength = rootHost.split('.').length - 2; |
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.
As both middlewares need the root host's subdomain count, moving it to the config eliminates duplicates. This also allows to remove the rootHost constructor param and take it directly from the config.
I switched it to the config and also cleaned up the middleware parameters. |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
=======================================
Coverage 59.13% 59.13%
=======================================
Files 16 16
Lines 4667 4667
Branches 234 234
=======================================
Hits 2760 2760
Misses 1905 1905
Partials 2 2 ☔ View full report in Codecov by Sentry. |
@kay-is One final note (didn't notice this earlier) - PRs should be against |
The sandbox and ArNS middleware didn't work when the node was hosted on a subdomain.
This PR adds subdomain support to both middlewares, so the node doesn't require a dedicated apex domain for hosting.