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

feature: open browser on npm start #94

Closed
wants to merge 1 commit into from
Closed

feature: open browser on npm start #94

wants to merge 1 commit into from

Conversation

pbnj
Copy link

@pbnj pbnj commented Mar 12, 2017

When user types npm start, open a default browser to localhost:4000.

This is achieved by the opn npm package which is cross-platform and will work on Windows, Linux, & Mac.

@@ -155,4 +156,5 @@ MongoClient.connect(config.db, function(err, db) {
});
*/

opn("http://localhost:" + config.port);
Copy link
Member

Choose a reason for hiding this comment

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

@petermbenjamin Thanks for the PR. I see the convenience of automatically opening the app in the browser on starting the server. However, I am a bit unsure how this would work when the app is hosted on services such as heroku, especially as the opn command has localhost hardcoded. We can possibly add a logic to conditionally execute the opn command only if the hostname is localhost. Any thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

I completely agree.

I see two options:

  1. Pull out this feature into a separate npm script (e.g. "dev" : "npm start && opn http://localhost:4000") which users can start locally with npm run dev.
  2. Check if process.env.NODE_ENV === "development", then open a browser.

"grunt-npm-install": "^0.3.0",
"grunt-retire": "^0.3.12",
"mocha": "^2.4.5",
"opn": "^4.0.2",
Copy link
Member

Choose a reason for hiding this comment

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

Should this be moved to dependencies section as the code (server.js) has a direct dependency on this module?

Copy link
Author

Choose a reason for hiding this comment

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

Since opn is expected to ever be executed in a development environment, I think we should keep it as a dev dependency, even if code has a direct dependency on this module.

Typically, users running this locally would have to npm install before they npm start, so dev dependencies will be installed as well.

@binarymist
Copy link
Collaborator

How does this work within a container?
What happens if a browser tab already has nodegoat fetched?

@pbnj
Copy link
Author

pbnj commented Mar 19, 2017

@binarymist , I haven't thought of a container scenario.
I would presume that if it's a linux-based container without xdg-open built-in linux utility, then the opn command will simply fail, but the server.js should continue to run.
Obviously, I will need to test this to confirm.

As for what happens if a browser tab already has nodegoat fetched, opn would simply open another tab.

opn module simply detects the OS that it's executing within and calls the appropriate OS command to open a browser window based on the user's default preferences.
On macOS, opn module will call open command.
On Windows, opn module will call start command.
On Linux, opn module will call xdg-open command.

@binarymist
Copy link
Collaborator

Deployment via docker is one of the NodeGoat deployment strategies, as per the readme.
I can see a second browser window opening as more of an annoyance than anything.
I'm just struggling to see how this adds value?

@pbnj pbnj closed this Mar 19, 2017
@pbnj pbnj deleted the feat/open-browser branch March 19, 2017 03:14
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.

None yet

3 participants