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

Conversation

duncanbeevers
Copy link
Contributor

@duncanbeevers duncanbeevers commented Aug 7, 2017

If you name your ~/.hotel/servers files using non-uri characters, they aren't recognized by the exists middleware.

This PR allows intuitive, human-friendly naming of these files, and a more beautiful list of servers.
beauty-list

@duncanbeevers duncanbeevers force-pushed the decode-uri-component-hostname branch 2 times, most recently from c2ea9cd to a6a7c22 Compare August 7, 2017 08:29
@duncanbeevers duncanbeevers changed the title Decode URI entities in id param and hostname Separate server name and id, id uses hostname-valid encoding Aug 7, 2017
@duncanbeevers duncanbeevers changed the title Separate server name and id, id uses hostname-valid encoding Separate server name and id, use hostname-valid encoding Aug 7, 2017
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.

module.exports = function normalize(id) {
return id
? punycode
.encode(id)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

punycode won't mess with existing ASCII; it only ensures that non-ASCII characters get embedded into a hostname-valid space.

? punycode
.encode(id)
.toLowerCase()
.replace(/[^a-z0-9-.]/g, '-')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ensures that ASCII characters such as spaces, parentheses, commas, and so on, are encoded to a hostname-valid space.

.encode(id)
.toLowerCase()
.replace(/[^a-z0-9-.]/g, '-')
.replace(/-+/g, '-')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because so many characters are replaced by hyphen, any run of multiple hyphens is collapsed to a single hyphen.

@duncanbeevers
Copy link
Contributor Author

@typicode ping

@duncanbeevers
Copy link
Contributor Author

This PR has been updated to incorporate the changes from the UI refactor.

@@ -69,3 +69,7 @@ li > span {
p {
padding: 1rem;
}

.listLink {
line-height: 1em;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change prevents monitor names from collapsing.

Before

screen shot 2018-01-11 at 9 13 08 am

After

screen shot 2018-01-11 at 9 11 48 am

@@ -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?

.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.

package.json Outdated
@@ -103,6 +103,7 @@
"npm-run-all": "^4.1.2",
"pkg-ok": "^1.0.1",
"prettier": "^1.10.2",
"punycode": "^2.1.0",
Copy link
Owner

Choose a reason for hiding this comment

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

Could you move it to dependencies as it's used in the daemon?

@typicode
Copy link
Owner

Thanks for the PR :) (and the style fix)

I'd like to understand better, the idea is to be to have "pretty" names for files and automatically get valid domain?

Something like:

~/.hotel/servers/My Server (something).json # my-server-something.localhost

What does your projects directories look like? Do they have () as well or is it just something you want to have in hotel UI?

@duncanbeevers
Copy link
Contributor Author

@typicode The filenames represent the displayed values, like so:

❯ ls -1 ~/.hotel/servers/
Always Master (staging).json
Always Master.json
Auth (staging).json
Auth.json
PMP (desktop).json
PMP (tv).json
Style Guide.json
desktop (staging).json
desktop.json
hotel.json
reflector.json
tv.json

In some cases, parameterizable hotel servers would alleviate this, as many of the configurations are very similar to one another with the modification of one or two command-line flags.

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