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

Make react-chat-app easy to run for new developers #406

Open
wants to merge 5 commits into
base: next
Choose a base branch
from

Conversation

snickell
Copy link

@snickell snickell commented May 17, 2016

It took me about an hour to get my first working client/server example horizon app running. There's a lot of details to figure out, and the examples don't include .hz/config.toml files.

This makes one of the examples really easy to run, and points new developers toward that example.

I think this would save a lot of first time developers a lot of time.


This change is Reviewable

@snickell
Copy link
Author

The failing build looks like an unrelated race condition.

@deontologician
Copy link
Contributor

Hey @snickell thanks for the PR :)

As a rule, we don't commit config.toml files to the repo, since they contain token secrets. Even for example apps, this is a bad practice since people may be tempted to use the token secret in production not realizing it's in there.

So I'd say the instruction steps should tell the user to run hz init inside the example directory before running hz serve --dev

@deontologician deontologician added this to the Examples milestone May 19, 2016
@danielmewes
Copy link
Member

danielmewes commented May 19, 2016

@deontologician I noticed that hz serve refuses to run if there is no .hz directory, but if you create a .hz directory it starts up fine in development mode (even if the directory is empty and doesn't have a configuration file inside).
Do you think it would be ok if we added that directory to all examples and put just a short NOTE or README file inside (since we can't have have empty directories in git)?

Alternatively, would it be so bad to submit a config file with just the project-name in it?

@deontologician
Copy link
Contributor

Theoretically, we should allow a missing token_secret key if unauthenticated users are enabled. If we did that, it would be reasonable to add a bare config.toml. Actually that may work already, I'm not sure

@danielmewes
Copy link
Member

@deontologician It fails if token_secret isn't set, but I think the experience for the user is still fairly reasonable. It will print

serve failed with Error: No token_secret set! Try setting it in .hz/config.toml or passing it to the Server constructor.

so you can just pass it --token-secret foo and it will start up.

@deontologician
Copy link
Contributor

Ah, ok. I guess that should be ok then. So for this PR, would you just remove the token_secret line from the config.toml @snickell ?

@snickell
Copy link
Author

@deontologician no prob, though I think this user experience still leaves something to be desired for first-time developers.

I'd just like to say that I think horizon is really cool, and I'm over this step myself, but I'd really like horizon to succeed, so I'd like new developers to have really pleasant experiences with it. So if my input is helpful, great, and if not, totally ignore it and get on with working on stuff you think is more important!

Here's why I think having to set token_secret in config.toml isn't great for an example that many/most first-time developers will be running to figure out how to do "the next step":

  1. The file is hidden, which mildly increases both the annoyance of updating the file (e.g. a number of common editors don't show hidden files by default, which makes them annoying to update), and increases the chance of confusion just by virtue of not immediately seeing the file.
  2. When I hit little niggly bumps like this starting out with a library or framework, I start thinking "man, that's a lot of arbitrary hoops to jump through, is this framework going to be annoying?". Also, I start wondering "Ok, I'm game to do this silly step a computer could do for me... but is this the last one or is this going to be hours of jumping through hoops?".
  3. As a first time developer, I have no idea how to set token-secret, what valid values are, etc, so now I have to go look this up, or guess that I can copy one from a fresh init, or... again, uncertainty.

Is there some way to have cake and eat it too here?

@deontologician
Copy link
Contributor

So theoretically, we could leave the token_secret out, and only require it once you enable authentication methods other than unaunthenticated (which would go hand in hand with #426 ). The issue is that adding a token afterwards, once you need authentication, is annoying, since toml files don't like to be programmatically modified (they have comments etc). So it's much easier for us to generate a token_secret for you when you create your app, that way it's guaranteed to use secure random bytes, and not be copy/pasted from the internet.

Those are the constraints as I see them. I think the easiest thing to attack might be adding the token secret to your config once you need authentication. It's annoying, but adding authentication is a step from "playing around" to "I'm doing this for real" so maybe it's not such a big deal

(Also, thanks for writing down your thoughts, we really appreciate the feedback)

@danielmewes
Copy link
Member

danielmewes commented May 20, 2016

We could also modify the error message from the server if no token is specified to read somewhat like this:

serve failed with Error: No token_secret set. Try setting it in .hz/config.toml or passing it to the server constructor.
Here is a random token that I have generated. You can pass it to `hz serve` by starting with this option:
`--token-secret ABCDEFG....`

@deontologician
Copy link
Contributor

Yeah, good error messages will be essential. I think we could probably offer to insert the config item into their config.toml. Maybe appending a line to the end of the file if it doesn't already have a token_secret key in it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants