JSRocksHQ / harmonic

The next static site generator
http://harmonicjs.com/
MIT License
283 stars 26 forks source link

`harmonic run` automatically open default browser #99

Closed UltCombo closed 9 years ago

UltCombo commented 9 years ago

It would save a bit of time if harmonic run opened localhost:port in the default browser. Perhaps harmonic run could also take a flag to not open a browser window.

jaydson commented 9 years ago

Great idea :+1:

jaydson commented 9 years ago

I started to work on this feature. You can check the following branch: https://github.com/es6rocks/harmonic/tree/open

Feature:
harmonic run opens the default browser pointing to the website URL (localhost + port) harmonic new_post opens the markdown file for the post(s) in the default text-editor harmonic new_page opens the markdown file for the page(s) in the default text-editor
This feature is configurable by changing the allowopen option in harmonic.json

By default, for new websites, allowopen is false. I changed the harmonic.json template to have the allowopen option.

TODO: Do not use this feature in our tests, because setting allowopen to true will open the files when we try to run or create a file.

UltCombo commented 9 years ago

@jaydson Very nice work! :+1: Working nicely here. :smile:

Couple nitpicks:

jaydson commented 9 years ago

Thanks for reviewing @UltCombo :+1: allow_open makes a lot of sense, i'll change it. "Per-developer preference" seems to be the best approach, i'll also do it.

jaydson commented 9 years ago

Actually, let's keep just the "per-developer preference" option, in the CLI with --no-open and it opens by default.

UltCombo commented 9 years ago

Actually, let's keep just the "per-developer preference" option, in the CLI with --no-open and it opens by default.

:+1:

For those that don't like specifying flags every time, we could support supplying configs through a .harmonicrc file, using the rc package. Of course, we can leave that for the future when we have more per-developer preferences. :smiley:

jaydson commented 9 years ago

Cool. And where you think this .harmonicrc file must stay? One .harmonicrc per generated Harmonic project? One .harmonicrc per Harmonic installation?

UltCombo commented 9 years ago

It may be placed in any of the standard places, the rc package handles loading those automatically. The user usually creates the file manually where prefer.

UltCombo commented 9 years ago

But I believe we don't have to worry about it for now, can open another issue to track this.

jaydson commented 9 years ago

I need some help here. I'm not finding any way to run the tests, and it always fails because by default we're opening files/urls. Any thoughts? I have pushed the current code to the branch open.

UltCombo commented 9 years ago

Is this the failing test?

  1) CLI should create and build a new post:
     Uncaught Error: ENOENT, no such file or directory '/path/to/harmonic/dist/test/site/src/posts/en/new-post-test.md'

For some reason it is failing in the master branch now too.

UltCombo commented 9 years ago

Ah wait never mind, I had added the --no-open flag to the tests but it was failing because Commander.js was not configured to accept it:

error: unknown option `--no-open'

I checked out to the master branch without resetting the test files so the error persisted.

UltCombo commented 9 years ago

@jaydson should be fixed now, please review/test https://github.com/es6rocks/harmonic/commit/ea3ec9a4c042872b72949bf44864b5e252d6dbd3 Feel free to merge too. =]

UltCombo commented 9 years ago

Pushed a few more fixes to the open branch, please take a look when you have time.

jaydson commented 9 years ago

Awesome! Thanks @UltCombo I'll check it asap.

UltCombo commented 9 years ago

@jaydson I've done some testing and everything looks ok. Please test it as well (check if build is passing etc.) then we can merge. =]

jaydson commented 9 years ago

I'm still having similar issues:

1) CLI should display an error for unknown commands: Uncaught AssertionError: expected 'child_process: customFds option is deprecated, use stdio instead.\nmodule.js:338\n throw err;\n ^\nError: Cannot find module \'gulp-6to5/node_modules/6to5/polyfill\'\n at Function.Module._resolveFilename (module.js:336:15)\n at Function.Module._load (module.js:278:25)\n at Module.require (module.js:365:17)\n at require (module.js:384:17)\n at Object. (/home/jaydson/Workspace/harmonic/dist/bin/cli/harmonic.js:7:1)\n at Module._compile (module.js:460:26)\n at Object.Module._extensions..js (module.js:478:10)\n at Module.load (module.js:355:32)\n at Function.Module._load (module.js:310:12)\n at Function.Module.runMain (module.js:501:10)\n' to contain 'foobarbaz' at Assertion.fail (/home/jaydson/Workspace/harmonic/node_modules/should/lib/assertion.js:113:17) at Assertion.prop.(anonymous function) as containEql at /home/jaydson/Workspace/harmonic/dist/test/main.js:44:21 at ChildProcess.exithandler (child_process.js:739:5) at ChildProcess.emit (events.js:110:17) at maybeClose (child_process.js:1000:16) at Process.ChildProcess._handle.onexit (child_process.js:1072:5)

events.js:85 throw er; // Unhandled 'error' event ^ Error: 1 test failed.

UltCombo commented 9 years ago

The error you're getting is:

Cannot find module \'gulp-6to5/node_modules/6to5/polyfill\'

If you run npm install in the Harmonic repository, does it install gulp-6to5 in the node_modules dir?

If not, then perhaps you have gulp-6to5 in a node_modules dir higher up in your directory hierarchy, in that case we can/should add 6to5 as a direct dependency.

UltCombo commented 9 years ago

I'll open a PR to see if Travis throws any error.

jaydson commented 9 years ago

Yes, i have gulp-6to5 in the node_modules dir.

UltCombo commented 9 years ago

Ohh, you most likely have 6to5 in a node_modules dir higher up in your directory hierarchy then, so npm doesn't install it again.

In that case we should add a direct dependency to 6to5 in order to prevent running into these cases. Though, we only need the polyfill, so there might be a better option.

UltCombo commented 9 years ago

Nope, Travis build is also failing. Let's continue the discussion at #104