Added config to turn off browser sync

[abbott567]

Every time I make a new prototype I have to keep deleting out the browser sync code. I've added an option to the config file to make disabling it much easier in future. It's left enabled by default so there is no change to the way the kit works out the box.

[abbott567]

Hey Tommo, yeah I worked out it was using the -50 but the autofill on the browser just always wanted to go to :3000 😂 - It just got frustrating. You're right, there might be no need for this PR, just figured for the sake of 2 lines of code it might have been nice to have an option to easily disable it.

[NickColley]

@abbott567 just digging into the root problem, what's the reason you're disabling browserSync?

[abbott567]

I find it inconsistent. sometimes it works without issues, but quite often it seems to break, particularly when editing and compiling SASS, so I still find myself refreshing the page anyway. I think it would be nice to have the option rather than forcing people to use it. i guess for me there is no user need for browser sync. Perhaps I'm the only one though haha

[tvararu]

What about using the Browser Sync ui option? @tsmorgan

[tsmorgan]

I'm not sure. The UI bit is always something I've ignored mostly. How do you mean though? As a way to turn it off? Or just to surface it and allow people more choice about how browsersync works for them?

[tvararu]

@tsmorgan I mean surfacing it by enabling it, as it's currently turned off, for example:

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

[edwardhorsford]

It feels awkward to have another way of doing something.

[abbott567]

As @tvararu pointed out in the PrototypeKit_Dev Slack channel, you'd have to navigate to the BrowserSync UI and disable it each time you ran the node server. As somebody who wants to disable it completely, surfacing the UI doesn't meet my need. Doesn't mean I'm against surfacing the UI, I just think they are two different things.

[tsmorgan]

My preference was always for this kind of thing making it clear to users that browsersync was running but on another port to the normal prototype. You can see Joe's objections there which still I think are valid, but since I have seen a number of comments which make me think that there's people who would probably appreciate the choice more.

[joelanman]

I think this config option looks like a good solution, will merge tomorrow unless there are any objections

[joelanman]

Just wondering if we can make this more consistent with the way other config is handled. Unfortunately we already seem to have 2 ways, this would be a third.

https://github.com/abbott567/govuk_prototype_kit/blob/f71973ecd1b816f8f23a9e7d47d725af0e868a7e/server.js#L21-L29

1: 'look in environment variables first, then config', then make it lowercase. Leave as string (useAuth)
2: Just check config, and convert to boolean (useDocumentation)

[abbott567]

Hey Joe,

I'm not sure I understand. I made 'true' as a string as the others were like that in the config file, but I would have just made it a boolean.

Do you mean, assign it to a variable in the server file and then use that in the or statement?

[joelanman]

Sorry wasn't very clear yeah I think it'd be good to at least stick to 'here's where we set all our config vars' at the top

[abbott567]

I'll do that =) do you think I should make the string to boolean changes at the same time? or should that be it's own issue / PR?

[joelanman]

so we already have both methods being used, so I dont mind as long as it looks like one of them :) we can consolidate from 2 methods to 1 later

[abbott567]

Hi @joelanman, I have added a new commit that assigns config.useBrowserSync to a variable, runs toLowerCase and then references the variable in the statement instead.

[joelanman]

thanks @abbott567 !