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

Add BrowserSync #1024

Closed
wants to merge 1 commit into from
Closed

Conversation

sedubois
Copy link
Contributor

@sedubois sedubois commented Sep 23, 2016

Just noting that besides adding the basic browserSync functionalities, this switches tunnelling from ngrok to browserSync's built-in feature (uses localtunnel), whose behaviour might be slightly different. For example localtunnel doesn't work with my VPN, but OTOH it allows the tunnel to be nicely integrated within browserSync's dashboard, and the tunnel's URL can be configured for free.

If agreed and if wanting to clean up, ngrok could be removed altogether, which would require updating pagespeed.js (need to launch server, wait for browserSync init, then pass tunnel URL to PageSpeed Insights).

@sedubois sedubois force-pushed the browsersync branch 2 times, most recently from c55e522 to ccf636f Compare September 23, 2016 14:13
@amilajack
Copy link
Member

I havent used browserSync much. What is the advantage/difference between it and react-hot-reloader?

@sedubois
Copy link
Contributor Author

sedubois commented Sep 23, 2016

Provides a nice dashboard, single-click to reload page on all devices, indicator when your pages get disconnected/reconnected from network, keep page navigation and scrolling in sync across all the browsers, etc. I suggest to try out to get a feel of it.

@sedubois
Copy link
Contributor Author

Also syncs clicks across devices, so you click a button on one of them and it does the same on all others. Same thing for form input.

@sedubois
Copy link
Contributor Author

Also visual CSS outlining, network throttling, remote debugging. Throttling kinda shows that this boilerplate isn't usable below 4G. Didn't try the remote debugging.

@andykenward
Copy link
Contributor

andykenward commented Sep 23, 2016

You arent running production built code when you run the npm start script you changed.

It is only worth using the network throttling when running production built code. This boilerplate is totally usable below 4G.

@sedubois
Copy link
Contributor Author

OK, good to know. With more work then maybe browserSync could be launched for either dev or prod.

@gihrig
Copy link
Contributor

gihrig commented Sep 23, 2016

... browserSync's built-in feature (uses localtunnel), ... For example localtunnel doesn't work with my VPN ...

The main value I get from ngrok is the ability easy share work in development with clients and colleagues across the internet. I would not want to lose that ability.

@sedubois
Copy link
Contributor Author

@gihrig localtunnel doesn't work for you?

@gihrig
Copy link
Contributor

gihrig commented Sep 23, 2016

@sedubois I never tied it. Your remark (quoted above) led me to believe that it would not.

Can you explain more about the differences between ngrok and localtunnel?

Why/how does localtunnel not work with your VPN?

@sedubois
Copy link
Contributor Author

sedubois commented Sep 23, 2016

With my VPN enabled, localtunnel doesn't work whereas ngrok does. But I suspect not many people have such a setup? With VPN disabled, localtunnel works fine, I can access my machine from the internet.

OTOH as mentioned, localtunnel is integrated within browserSync's dashboard, and allows custom subdomains, which can be handy when sharing URLs with clients. In ngrok, this is a paid feature.

Note that I'm not fully familiar with either of these tunnel providers. I casually used ngrok in the past and recently discovered localtunnel as it is provided within browserSync.

@amilajack
Copy link
Member

amilajack commented Sep 23, 2016

Note that PR's should be made to the dev branch instead of master.

@mxstbr
Copy link
Member

mxstbr commented Sep 23, 2016

which would require updating pagespeed.js

Note: #894 will probably be merged, so this is probably not a concern at this point.

I have never personally used BrowserSync, so I'll have to play with it and see what it feels like. (not sure when I have the time, but I'll try to soon! Currently on vacation in 🗽) Question: Does this replace hot reloading?

@amilajack
Copy link
Member

amilajack commented Sep 23, 2016

Hot reloading is a more modern solution for updating the UI's during development. They can't save state. Instead, they reload the entire page. Its impossible to not lose the state if the page is reloaded.

@amilajack amilajack closed this Sep 23, 2016
@mxstbr
Copy link
Member

mxstbr commented Sep 24, 2016

I think closing this was a bit premature, BrowserSync does have value.

Contrary to hot-reloading, it syncs button clicks and interactions across pages, which makes cross-device testing very easy and quick.

The question I have if those two techniques could be used alongside each other, or if they're completely separate?

@sedubois
Copy link
Contributor Author

sedubois commented Sep 24, 2016

Does this replace hot reloading?

@mxstbr browserSync can be configured (as in this PR) to proxy webpack, the latter then handling hot reloading.

https://github.com/Browsersync/recipes/tree/master/recipes/webpack.react-hot-loader
https://github.com/mxstbr/react-boilerplate/pull/1024/files#diff-8442264f3fb929d20cf95f311ccbc48dR13

@mxstbr
Copy link
Member

mxstbr commented Sep 24, 2016

So, essentially, the only difference here to the current version is that if you open the webpage on multiple devices at the same time, all the interactions sync, do I have that right?

If so, that's pretty cool I think 😊

@sedubois
Copy link
Contributor Author

@mxstbr yes 😊 plus some nice-to-have tools like CSS outlining, remote debug, network throttling. And I switched tunnelling from ngrok to browserSync's, but that part isn't required, could be rolled back.

@sedubois sedubois changed the base branch from master to dev September 26, 2016 08:58
@sedubois sedubois mentioned this pull request Sep 28, 2016
@mxstbr
Copy link
Member

mxstbr commented Sep 28, 2016

I like the code I've seen (though I haven't done an in-depth review yet), the thing that's holding me back here is that I haven't tested this yet so I'm not sure how awesome this is.

I'm pretty busy at the moment, so if somebody who's never used BrowserSync could try this out and report back how useful it is that'd be amazing!

@sedubois sedubois force-pushed the browsersync branch 2 times, most recently from ee6a76a to aace146 Compare September 28, 2016 10:31
@GGAlanSmithee
Copy link
Contributor

if somebody who's never used BrowserSync could try this out and report back how useful it is that'd be amazing!

I've never used browsersync before, so I took this opportunity to test it out. I ran the app in a couple of browser sessions, as well as on my Nexus phone and the syncing works great. The combination of browsersync with HMR makes for a great developer experience. Especially if you target phones, as you can navigate it remotely from your desktop.

note: I am using cloud9 ide, so I had to add these lines since c9 only allows for ports 8080, 8081 and 8082. With this setup you get HMR at 8080, browsersync+HMR at 8081 and the browsersync dashboard at 8082.

@sedubois
Copy link
Contributor Author

@GGAlanSmithee thanks, I updated the code to set up ports correctly even when changing the default. Feel free to try again.

@GGAlanSmithee
Copy link
Contributor

@sedubois tested again with your changes and got error

RangeError: "port" option should be >= 0 and < 65536: 80801

this happends because port is a string, so port+1 performs a string concatenation rather than an addition of integers. parseInt(port, 10) + 1 and parseInt(port, 10) + 2 fixes this.

Other than that, this is working as expected 👍

@sedubois
Copy link
Contributor Author

Thanks @GGAlanSmithee, I fixed again :)

@bsr203
Copy link

bsr203 commented Oct 3, 2016

About hot reloading, please read this comment gaearon/react-hot-boilerplate#97 (comment)

and the priorities are

Here is my current plan about adding hot reloading to React:

 Make Create React always refresh the page on change so people learn to trust it and stop pressing “refresh” themselves.

I came here as well to switching back from hot reloading to browser-sync.

or it could be how it is done without BrowserSync
https://github.com/facebookincubator/create-react-app/blob/master/packages/react-dev-utils/webpackHotDevClient.js

@GGAlanSmithee
Copy link
Contributor

@bsr203 thanks for linking to that issue, that is interesting indeed. Hopefully they will integrate this with react some time in the future and hide it under a development flag, much like propTypes checks are done.

Do you know if browser-sync can fully replace hmr?

@bsr203
Copy link

bsr203 commented Oct 3, 2016

@GGAlanSmithee to be honest, I am planning to move away from react-router, as this library works great with relay and migration was quite smooth. The coe is clean, and minimal.

A talk by the author: https://www.youtube.com/watch?v=Pj-RakjfHDI#t=33m15s

They also have a hot reloading example. gonna try it today.

@sedubois
Copy link
Contributor Author

sedubois commented Oct 18, 2016

So AFAIK, this browserSync works quite great with the exception of keyboard events. E.g in the example app there's a preventDefault which might be why it doesn't work. There's an issue about that as I described here, but no progress since 1.5 years.

So not for me to say if that's a blocker. Just noting that BrowserSync is still "pretty" popular, 1 million package downloads per month.

BrowserSync has other features e.g override locally the contents of a remote website, CSS outlining, network throttling, etc. Maybe in the future Webpack can offer all that.

@sedubois
Copy link
Contributor Author

sedubois commented Oct 18, 2016

Just got a reply from BrowserSync: they intend to improve keyboard syncing (no time frame specified).

@coveralls
Copy link

coveralls commented Oct 27, 2016

Coverage Status

Coverage remained the same at 98.958% when pulling a207987 on sedubois:browsersync into d4e0052 on mxstbr:dev.

@mxstbr
Copy link
Member

mxstbr commented Nov 2, 2016

To be honest, I don't think I really want to merge something by default that doesn't even work for the tiny example app. That being said, I definitely see the value of BrowserSync, so when things improve on this front I'd love to add it in the future!

Thanks for this great reference PR, sorry for the slow response here!

@mxstbr mxstbr closed this Nov 2, 2016
@sedubois sedubois deleted the browsersync branch November 2, 2016 09:59
@lock
Copy link

lock bot commented May 29, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants