Skip to content

Adds exception to '@' case - #59

Open
immjs wants to merge 3 commits into
bmullan91:masterfrom
immjs:patch-1
Open

Adds exception to '@' case#59
immjs wants to merge 3 commits into
bmullan91:masterfrom
immjs:patch-1

Conversation

@immjs

@immjs immjs commented Mar 25, 2021

Copy link
Copy Markdown

Thought it would be a nice addition to add root domains

Thought it would be a nice addition to add root domains

@bmullan91 bmullan91 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the PR 🙌 - can you add some colour to help me better understand why people would do this, it seems nonintuitive to me. If we are to include this we'll need tests + a readme update to go along with it.

immjs added 2 commits March 27, 2021 22:37
i think that's how you do it
@immjs

immjs commented Mar 27, 2021

Copy link
Copy Markdown
Author

o/ is this good? I'm not very common with tests and socializing with other humans

@immjs

immjs commented Mar 27, 2021

Copy link
Copy Markdown
Author

can you add some color to help me better understand why people would do this

Because sometimes you just want to handle the root domain, not forcibly the other ones

for instance, if I were to choose for 'api.example.com/about', id rather throw a 404 that would make sense to the dev than a successful request that would confuse them

@immjs

immjs commented Apr 28, 2021

Copy link
Copy Markdown
Author

Hello? I really want this PR to get merged, it's gonna facilitate a lot of things for me and for other people asw (I hope) @bmullan91

@immjs

immjs commented Oct 15, 2022

Copy link
Copy Markdown
Author

Hi!

So, to clarify, you can't go into any @ subdomain because in DNS it just means root.

Since it is the norm, I would expect to see it here as well

@danielamitay

danielamitay commented Aug 21, 2024

Copy link
Copy Markdown

I know it's been a long time since this PR, but I actually have this usecase as well. My original code:

app.use(subdomain('api', apiRouter));
app.use(wwwRouter);

Means that if anyone visits the (nonexistent) admin subdomain, it will be handled by the wwwRouter, which is not intended/desired.

To deal with this, I have had to add a specific check for unknown subdomains, like the following:

app.use(subdomain('api', apiRouter));
// Serve 404 for unknown subdomains
app.use((req, res, next) => {
  if (req.subdomains.length === 0) {
    next()
  } else {
    res.sendStatus(404)
  }
})
app.use(wwwRouter);

If the change from this PR was introduced, I could simplify to something like:

app.use(subdomain('api', apiRouter));
app.use(subdomain('www', wwwRouter));
app.use(subdomain('@', wwwRouter));

Unhandled/unknown subdomains would get the standard 404 behavior, and/or I could add my own global 404 after these statements, without having to do the req.subdomains check that I am currently doing.

@immjs

immjs commented Aug 21, 2024

Copy link
Copy Markdown
Author

Honestly, this package looks and feels abandoned. You're better off making a fork and publishing it under an org with your name (iirc I have that under @immjs/express-subdomain )

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.

3 participants