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

Use Public Suffix for homarr_domain #2147

Closed
wants to merge 1 commit into from

Conversation

ColinHebert
Copy link

@ColinHebert ColinHebert commented Oct 6, 2024

Category

Feature

Overview

Changes the homarr_domain detection to take the complete domain rather than the top domain only.

When the domain of homarr is homarr.myMachine.domain.tld, myMachine.domain.tld is detected as publicSuffix

This makes use of allowPrivateDomains to detect the suffix as documented on https://www.npmjs.com/package/tldts

tldts.parse('https://spark-public.s3.amazonaws.com/dataanalysis/loansData.csv');
// { domain: 'amazonaws.com',
//   domainWithoutSuffix: 'amazonaws',
//   hostname: 'spark-public.s3.amazonaws.com',
//   isIcann: true,
//   isIp: false,
//   isPrivate: false,
//   publicSuffix: 'com',
//   subdomain: 'spark-public.s3' }

tldts.parse(
  'https://spark-public.s3.amazonaws.com/dataanalysis/loansData.csv',
  { allowPrivateDomains: true },
);
// { domain: 'spark-public.s3.amazonaws.com',
//   domainWithoutSuffix: 'spark-public',
//   hostname: 'spark-public.s3.amazonaws.com',
//   isIcann: false,
//   isIp: false,
//   isPrivate: true,
//   publicSuffix: 's3.amazonaws.com',
//   subdomain: '' }

Issue Number

Related issue: #2146

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hi 👋. Thank you for making your first contribution to Homarr. Please ensure that you've completed all the points in the TODO checklist. We'll review your changes shortly.

@Meierschlumpf
Copy link
Collaborator

I would suggest to add a new variable, otherwise this is a breaking change

@SeDemal
Copy link
Collaborator

SeDemal commented Oct 6, 2024

You're also have to add the var in the informational pop up where this element can be used (Mostly thinking of the apps setting's External address entry).

I also would try to find a different name for the var as "domain" is the really non-descriptive way of addressing the address, and is normally followed or prefaced with a descriptor to better isolate the part, and alone would be common for the TLD - SLD combination only. (In this case, it was also based on tldts very definition of it listed further down from your example)

For your var I would personally call it "Parent-Domain" or "Parent" for the short version. So "homarr_parent" feels more fitting here.

My suggestion:

        return appType.behaviour.externalUrl
          .replace('[homarr_base]', `${window.location.protocol}//${window.location.hostname}`)
          .replace('[homarr_hostname]', tldts.getHostname(window.location.href) ?? '')
          .replace('[homarr_domain]', tldts.getDomain(window.location.href) ?? '')
          .replace('{homarr_parent]', tldts.getPublicSuffix(window.location.href, { allowPrivateDomains: true }) ?? '')
          .replace('[homarr_protocol]', window.location.protocol.replace(':', ''))
          .replace('[homarr_port]', window.location.port);

remove parsedUrl and change the bracket under from that to "window.location".

Edits: I'm a bit blind don't mind me.

@JoJenH
Copy link

JoJenH commented Oct 14, 2024

But then it would seem like an infinite number of new variables would need to be defined for multiple domains.
It is recommended to use a list-like approach to get the elements. Using index for different level domain name, like homarr_domain[4] for homarr.myMachine.domain.tld, homarr_domain[2] to get the domain.tld. Or some other way.
Also, we want to add a homarr_port variable to get the port, which is important in cases where different hostnames or URIs are used to differentiate services on the same port(different ports for internal and external networks).

@SeDemal
Copy link
Collaborator

SeDemal commented Oct 18, 2024

@ColinHebert Are you still working on this or would you rather we take care of it?

@SeDemal
Copy link
Collaborator

SeDemal commented Nov 12, 2024

Closing the PR since it was voted in this PR #2189 that we won't be pursuing URI's with variable and they will be removed altogether.
I am guessing they might stay as is in v0.15 but won't in V1.0.

@SeDemal SeDemal closed this Nov 12, 2024
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.

4 participants