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

Adding browser-sync to the prototyping kit. #193

Merged
merged 6 commits into from
May 27, 2016

Conversation

tsmorgan
Copy link
Contributor

This PR is to replace #144 and #149 but still in response to issue #84.

After discussion with @joelanman I've reverted to adding a callback to the findAvailablePort function and then start browser-sync in server.js (or not if in 'production'). I've also incorporated Joe's updated browser-sync config options into this new PR.

(Only tested on MacOSX 10.11.4 so far though)

@joelanman
Copy link
Contributor

nice, but the terminal output looks a bit confusing:

image

do you think we can make it cleaner/simpler for our users?

@joelanman
Copy link
Contributor

logLevel: "error" seems to hide that output, and then we can replace our own "listening on port 3000" with "listening on port 3050" etc

      browserSync({
        proxy:'localhost:'+port,
        port:port+50,
        ui:false,
        files:['public/**/*.*','app/views/**/*.*'],
        ghostmode:false,
        open:false,
        notify:false,
        logLevel: "error"
      });

@tsmorgan
Copy link
Contributor Author

@joelanman good idea, I've updated it.

@joelanman
Copy link
Contributor

joelanman commented May 21, 2016

great! It's a tiny thing, but I wonder if we should set the kit to run on port 2950 (config.port-50), then browser-sync runs on the config port of 3000. Otherwise config doesn't match where you "should" be looking at the kit, and people are going to have to learn a new port, 3050.

I've checked online and can't see anything obvious that runs in this range (2950 - 3000)

@tsmorgan
Copy link
Contributor Author

tsmorgan commented May 21, 2016

I see your point. I think I've managed that. Would appreciate a sanity check; for some reason it took me a little while to get my head around how to do it. :)

My only worry here is that findAvailablePortwon't be checking 2950. It should still work because legacy kits will be running on >=3000 and new browser-sync kits will have browser-sync running on those ports but if, for some reason, someone does have something on ports 2950-2999 things will break in a way which might not be dead easy to spot (given we're obscuring the real port now).

I wonder whether it might be more transparent to acknowledge browser-sync is running and sort of say "To use browser-sync, use THIS URL instead". People who don't know/care about browser-sync could use either URL and they'll get what they expect, and people familiar with browser-sync will understand the purpose of the second URL.

@@ -35,7 +35,7 @@ exports.basicAuth = function(username, password) {
};
};

exports.findAvailablePort = function(app){
exports.findAvailablePort = function(app,callback){
Copy link
Contributor

@NickColley NickColley May 21, 2016

Choose a reason for hiding this comment

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

tiny nit (sorry), spaces between params 👼

@tsmorgan
Copy link
Contributor Author

@joelanman Take a look at at this last commit. I've gone with starting at 3000 and just putting browser-sync on port+1. I've included an extra message below the normal one like:

Listening on port 3000     url: http://localhost:3000
Browsersync on port 3001   url: http://localhost:3001

Any new kit starting up will check both these ports and end up picking 3002 but at least it will have checked both ports and so isn't assuming an empty port without checking.

I know this is a move away from the seamlessness that you're advocating above but I think there's value here in showing the seams! :-)

@joelanman
Copy link
Contributor

I'll have a think about an approach that can work, but just wondering what the value is showing the seam here? If someone is new to programming and the kit, what should they do after running npm start? If they were to ask why they have two versions?

@tsmorgan
Copy link
Contributor Author

Hey @joelanman. OK I've reverted that last change so we've got express listening on port 2950 and Browsersync on port 3000, with no mention of it in the output. If you're happy to go ahead with that then that's cool.

@joelanman joelanman merged commit d00b3b2 into alphagov:master May 27, 2016
@joelanman joelanman mentioned this pull request May 27, 2016
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