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

Separate server name and id, use hostname-valid encoding #216

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
"once": "^1.3.2",
"please-upgrade-node": "^3.0.1",
"pug": "^2.0.0-rc.4",
"punycode": "^2.1.0",
"respawn": "^2.4.1",
"selfsigned": "^1.10.1",
"server-ready": "^0.3.1",
Expand Down
1 change: 1 addition & 0 deletions src/app/Store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export interface ILine {
}

export interface IMonitor {
name: string,
cwd: string
command: string[]
status: string
Expand Down
8 changes: 5 additions & 3 deletions src/app/components/Content/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,11 @@ class Content extends React.Component<IProps, {}> {
}}
>
<div className="content-bar">
<span>
<Link id={store.selectedMonitorId} />
</span>
{monitor && (
<span>
<Link id={store.selectedMonitorId} name={monitor.name} />
</span>
)}
<span>
<button
title="Clear output"
Expand Down
10 changes: 6 additions & 4 deletions src/app/components/Link/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@ function href(id: string) {
}

interface IProps {
id: string
className?: string,
id: string,
name: string
}

export default function Link({ id }: IProps) {
export default function Link({ className, id, name }: IProps) {
return (
<a href={href(id)} target="_blank" onClick={e => e.stopPropagation()}>
{id}
<a className={className} href={href(id)} target="_blank" onClick={e => e.stopPropagation()}>
{name}
</a>
)
}
6 changes: 3 additions & 3 deletions src/app/components/Nav/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function Nav({ store }: IProps) {
onClick={() => store.selectMonitor(id)}
>
<span>
<Link id={id} />
<Link className="listLink" id={id} name={monitor.name} />
</span>
<span>
<Switch
Expand All @@ -63,9 +63,9 @@ function Nav({ store }: IProps) {
<h2>proxies</h2>
<ul>
{Array.from(proxies).map(([id, proxy]) => (
<li key={id}>
<li key={name}>
<span>
<Link id={id} />
<Link className="listLink" id={id} name={id} />
</span>
</li>
))}
Expand Down
22 changes: 13 additions & 9 deletions src/daemon/group.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const afterAll = require('after-all')
const httpProxy = require('http-proxy')
const serverReady = require('server-ready')
const log = require('./log')
const normalize = require('./normalize')
const tcpProxy = require('./tcp-proxy')
const daemonConf = require('../conf')
const getCmd = require('../get-cmd')
Expand Down Expand Up @@ -61,15 +62,18 @@ class Group extends EventEmitter {
return this._list[id]
}

add(id, conf) {
add(name, conf) {
conf.name = name
const id = normalize(name)

if (conf.target) {
log(`Add target ${id}`)
log(`Add target ${name}`)
this._list[id] = conf
this._change()
return
}

log(`Add server ${id}`)
log(`Add server ${name}`)

const HTTP_PROXY = `http://127.0.0.1:${daemonConf.port}/proxy.pac`

Expand Down Expand Up @@ -97,7 +101,8 @@ class Group extends EventEmitter {

const mon = respawn(command, {
...conf,
maxRestarts: 0
maxRestarts: 0,
name
})

this._list[id] = mon
Expand Down Expand Up @@ -145,7 +150,8 @@ class Group extends EventEmitter {
this._change()
}

remove(id, cb) {
remove(name, cb) {
const id = normalize(name)
const item = this.find(id)
if (item) {
delete this._list[id]
Expand Down Expand Up @@ -204,10 +210,8 @@ class Group extends EventEmitter {
exists(req, res, next) {
// Resolve using either hostname `app.tld`
// or id param `http://localhost:2000/app`
const tld = new RegExp(`.${daemonConf.tld}$`)
const id = req.params.id
? this.resolve(req.params.id)
: this.resolve(req.hostname.replace(tld, ''))
const tld = new RegExp(`\\.${daemonConf.tld}$`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous regexp here wasn't enforcing a dot.

const id = this.resolve(req.params.id || req.hostname.replace(tld, ''))

// Find item
const item = this.find(id)
Expand Down
12 changes: 12 additions & 0 deletions src/daemon/normalize.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
const punycode = require('punycode') // eslint-disable-line node/no-deprecated-api

module.exports = function normalize(name) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why the ternary, have you spotted a case where it can can be undefined or is it for safety?
Could you write some tests for normalize or add a comment with examples of inputs and expected outputs?

return name
? punycode
.encode(name)
.toLowerCase()
.replace(/[^a-z0-9-.]/g, '-')
.replace(/-+/g, '-')
.replace(/-$/, '')
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure to understand what they're for?

.replace(/-+/g, '-')
.replace(/-$/, '')

Copy link
Contributor

Choose a reason for hiding this comment

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

It’s to ensure that multiple invalid characters only get replaced by one -, and to remove trailing -s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is just for tidiness sake. In the examples I posted server names include whitespace followed by non-alpha-numeric characters resulting in double hyphens.

It looks a little cleaner to me to tidy them, but the code I wrote mostly applies to my own naming scheme. For example, it doesn't remove leading hyphens, but that's only because I'd never named a server something that would result in a leading hyphen.

I'm happy to simplify this entire section and simply use the pure punycode replacements. That seems less likely to result in id collisions where multiple similarly-named servers normalize to the same server id.

: name
}