Browser sync #145

davidmpaz · 2017-08-23T11:57:46Z

This is the initial PR to fix #2

Please make a code review of this and to look at how was API usage proposed.

Unfortunately this can not be merged yet because of BrowserSync/browser-sync#1416

Just posted to be able to make reference in that task about this code

Lyrkan

Really quick review, I'll probably read all of that with more attention when I find some time to do so.

Lyrkan · 2017-08-23T13:00:21Z

index.js

+     * @param {function} browserSyncOptionsCallback
+     * @return {publicApi}
+     */
+    enableBrowserSync(backendUrl, paths, browserSyncOptionsCallback = () => {}) {


I'm wondering if it makes sense to have the backendUrl argument there. Couldn't we provide a default value for proxy/port and let people that need to override it use the browserSyncOptionsCallback?

Kind of the same remark about paths but I understand the need for that one a bit better.

Hi @Lyrkan,

it was just idiomatic thing like I said, I just thought that most use cases fit on this one, from my point of view it makes method signature to look nicer. We can change it, of course, although would be nice to think how will look method signature with paths as first argument only like this:

Encore.enableBrowserSync(['./*.twig', './*.php'], function(browserSyncOptions, pluginOptions) { browserSyncOptions.proxy = 'http://localhost:8080'; browserSyncOptions.port = '8080'; });

instead this:

Encore.enableBrowserSync( 'http://localhost:8080', ['./*.twig', './*.php'], function(browserSyncOptions, pluginOptions) {} );

Leaving only paths make me think we should remove it also and specify them in callback, or not?
Then we will have more boilerplate code in our callback always :(

some decision must be made regarding this one.

The questions we have to ask ourselves are:

will most users actually need to change the value of proxy and port?

will it be more frequent than having to modify paths?

will it also be more frequent than having to modify other options through the callback?

The same questions have to be asked for paths.

Then we will have more boilerplate code in our callback always :(

I'm not sure what you mean by that, can't we provide a sensible default value for each of them in browser-sync.js ?

will most users actually need to change the value of proxy and port?

Scenarios I have found are:

Usage of php built-in server. This I use heavily. Is normal to have couple of project running at same time with different ports.

Docker setup. Many colleagues use a docker container. In there, server is under vhost on default port 80/443 or just any port they choose to redirect to/from container, mainly because they also run several container at same time too.

Default server install (nginx/apache) with vhosts and port 80/443.

will it be more frequent than having to modify paths?

Regarding frequent changes on setup, honestly I don't know how could be, from my experience, it will change, even if the change it is not in repository committed, there will be cases for example where changing port for developing will happen.

I'm not sure what you mean by that, can't we provide a sensible default value for each of them in browser-sync.js ?

We can provide them, I did already when taking care of moving the variable initialization we talked before. But at the moment someone needs something different, which has been proven is happening most of the cases, then they will be helpless because of lack of configuration.

Actually discussing these ideas I have come to think that best compromise to avoid boilerplate code and to have still some flexibility is to use not the signature proposed initially but the one proposed below:

Encore.enableBrowserSync([prox url], [port], [paths to watch], [options callback]);

Leaving everything to configure in callback we would need to configure every time the same as we have now, only variations would be done in [proxy url], [port], and [paths to watch]*, rest would remain the same. The options we have now are:

{ proxy: [proxy url], port: [port], files: [ { match: [path to watch], fn: function(event, file) { if (event === 'change') { // get the named instance const bs = require('browser-sync').get('bs-webpack-plugin'); bs.reload(); } } } ] }

that is what I meant as boilerplate code, files array options, with a callback inside to execute on change event. All that will be the same always.

Usage of php built-in server. This I use heavily. Is normal to have couple of project running at same time with different ports.

Docker setup. Many colleagues use a docker container. In there, server is under vhost on default port 80/443 or just any port they choose to redirect to/from container, mainly because they also run several container at same time too.

Default server install (nginx/apache) with vhosts and port 80/443.

For the port these 3 cases should not be an issue if we avoid 80/443/8080/8443/8000 in our default values.

The proxy URL would not be an issue either since it'll nearly always be http://localhost:<dev server port> (but that implies that we can actually retrieve the port used by the dev server, see my other comment below).

But at the moment someone needs something different, which has been proven is happening most of the cases, then they will be helpless because of lack of configuration.

I agree that paths are a bit more complicated so maybe we should actually leave that one in parameters, but I'm not so sure about the proxy url and port:

Encore.enableBrowserSync([/* paths */], (bsOptions) => { bsOptions.port = 3002; });

Lyrkan · 2017-08-23T13:05:11Z

lib/plugins/browser-sync.js

+    let serverParts = url.parse(webpackConfig.browserSyncOptions.backendUrl, true);
+    let browserSyncConfig = Object.assign({
+        proxy: webpackConfig.browserSyncOptions.backendUrl,
+        port: serverParts.port || '80',


I'm not sure to understand why you use the port from backendUrl here.

Shouldn't proxy contain the URL of the dev server (so something like http://localhost:8080/ by default) and port the port Browsersync will listen to? Is there a case in which the port can be the same in both of them?

Also, as I said previously I don't think the backendUrl option is needed anyway. Removing it would also remove the need of the url dependency (which may look a bit strange and out-of-place for users).

Regarding the port settings

I think we must specify it at the moment we use the proxy option, the default is 3000 so not setting port could not work am afraid. Although to be honest I need to test it, did not think on that before.

I did decide to use proxy option because it cover more general configuration use case according to plugin use alongside with webpack-dev server and so on.

Regarding backendUrl to complement what said in previous comment, to remove url deps we could also pass port as parameter and them build proxy url concatenating both values. Signature would be:

Encore.enableBrowserSync('http://localhost', '8080', ['./*.twig', './*.php'], function(browserSyncOptions, pluginOptions) {} );

this of course in case we continue using parameters. If only callback used, then we don't need anything else.

I definitely agree about using proxy but according to the plugin page you linked the port in this case should be the one used by the dev-server (I don't know if we can retrieve it at this point, that would be great though...), whereas the one in the port property represents the port browser-sync will listen to:

{ // browse to http://localhost:3000/ during development host: 'localhost', port: 3000, // proxy the Webpack Dev Server endpoint // (which should be serving on http://localhost:3100/) // through BrowserSync proxy: 'http://localhost:3100/' },

If you use backendUrl both of them will end-up having the same value which doesn't seem right.

I think we are mixing approaches from plugin page. Here in current functionality, host option is not used. Only proxy, also, you made me spot a mistake about my approach, thanks! The port option is not required at all and nothing would brake if not specified, sorry for stating that before. I had the port option as a mean to run several project at once without conflicts.

So for example (I am not using port here, so default 3000 is used) by using the feature like this:

Encore.enableBrowserSyncPlugin('http://localhost:8091', [paths]);

this mean I have my php server running on: http://localhost:8091, which is the url I want to proxy with brower sync, in a separate terminal I ran: php -S 127.0.0.1:8091. Then in another terminal when I run encore dev-server I get an output like:

> encore dev-server Running webpack-dev-server ... Project is running at http://localhost:8080/ webpack output is served from /static/theme/ Content not from webpack is served from /home/shak/workspace/www 404s will fallback to /index.html DONE Compiled successfully in 66501ms 1:28:25 PM [BS] Proxying: http://127.0.0.1:8091 [BS] Access URLs: ------------------------------------- Local: http://localhost:3000 External: http://192.168.0.26:3000 ------------------------------------- UI: http://localhost:3001 UI External: http://192.168.0.26:3001 ------------------------------------- [BS] Watching files...

Project is running at http://localhost:8080/
webpack output is served from /static/theme/

this means that if you browse: http://127.0.0.1:8091/static/theme/some-css-file.css webpack dev server will give you the in memory compilation it has currently.

[BS] Proxying: http://127.0.0.1:8091

this is, as before out back end (php) server url, even if you don't use browser sync you can access this url.

[BS] Access URLs:
Local: http://localhost:3001
External: http://192.168.0.26:3001

these are the urls served by browser sync so you can have the live reloading as well as browser synchronization features, when you browse one of those, you are accessing your backend through browser sync, which in turn is watching the directories you gave before and when a change occur all browsers browsing that url will reload and show updated page.

does this explain better what functionality does currently?

Here in current functionality, host option is not used

It is, just with the "guessed" value (probably 192.168.0.26 based on your logs).

this means that if you browse: http://127.0.0.1:8091/static/theme/some-css-file.css webpack dev server will give you the in memory compilation it has currently.

That doesn't feel right. If you query 127.0.0.1:8091 browser-sync won't be reached since your PHP server is using that port.

Currently it seems that you have:

- browser-sync (3000) ---[proxy]---> php server (8091) --| - dev-server (8080)

I thought you were trying to implement this part of the documentation, which would basically look like:

-- browser-sync (3000) ---[proxy]---> dev-server (8080)

That also explains why you also added the "watch" part and why you would need to set the proxy option everytime you call Encore.enableBrowserSync(). However I'm wondering if that would be the most common use case of BrowserSync.

Let's say I'm an user working on a React project consuming an external API (so no PHP side-project). I see that there is an Encore.enableBrowserSync() method, what would I expect it to do?

Let's say I'm an user working on a React project consuming an external API (so no PHP side-project). I see that there is an Encore.enableBrowserSync() method, what would I expect it to do?

I see you point. I wanted to discover scenarios like this with this issue. I am not a React developer. Please could you explain how would be the use case and what would you expect from the API. Also please could you propose an API for it.

I thought you were trying to implement this part of the documentation

You are right, it was that one. Wondering now if I got it right? :)

To resume. I am having the feeling now that I implemented only an specific use case for the tool. If other uses cases deviate from current solution I think your initial proposition of having the option callback only would be the general common denominator. Not sure if worth the effort to implement more than that.

Thinking now whether this browser-sync integration issue could be best described in documentation, through snippets maybe, on how to use for the different uses cases.

I'm not really a React developer either, but the same idea applies to any other framework (Angular, Vue, ...).

I quickly added Browsersync to my preact demo to show what I meant by that: https://github.com/Lyrkan/encore-preact/tree/browser-sync

If you look at the webpack.config.js file you'll see that the value of proxy is the URL of the dev-server (unless you use --port). So when you run yarn encore dev-server you'll be able to browse http://localhost:3000/ and have access to both the dev-server (since it is proxied) and the features from Browsersync (events mirroring between browsers, network throttling, debug tools, etc.).

Lyrkan · 2017-08-23T13:10:14Z

lib/plugins/browser-sync.js

+    loaderFeatures.ensurePackagesExist('browsersync');
+    const BrowserSyncPlugin = require('browser-sync-webpack-plugin');
+
+    let browserSyncOptions = {};


const instead of let.

I would also have added the default settings there instead of an empty object: in other callbacks the objects that the user modifies are already set (which allows them for example to modify an option based on another one), which won't be the case here.

you are right. I will move initialization there.

Regarding let or const here, the object is changed in callbacks, would not this bring troubles ?

Using const won't be an issue if you don't want to re-assign the variable later (modifying its properties is OK though):

const obj1 = {}; const callback = (param) => { param.test = false; }; // allowed, then obj1.test === true obj1.test = true; // allowed, then obj1.test === false callback(obj1); // not allowed (Assignment to constant variable) obj1 = { test: false }

Lyrkan · 2017-08-23T13:10:39Z

lib/plugins/browser-sync.js

+    const BrowserSyncPlugin = require('browser-sync-webpack-plugin');
+
+    let browserSyncOptions = {};
+    let browserSyncPluginOptions = {};


Same remarks than for the browserSyncOptions variable.

Lyrkan · 2017-08-23T13:18:52Z

lib/plugins/browser-sync.js

+        ]
+    }, browserSyncOptions);
+
+    let browserSyncPluginConfig = Object.assign({


const instead of let

davidmpaz · 2017-08-25T06:36:27Z

Hi @Lyrkan,

thanks for the review. Just wanted to write to say I will go over all comments soon. It is just these couple of week I have no time for it. Next week everything will be back to normal.

cheers

weaverryan · 2017-09-13T18:55:12Z

Ping @davidmpaz! No rush - just wanted to remind you. I also just got back from several weeks away ;)

davidmpaz · 2017-09-13T19:29:57Z

Hey @weaverryan , @Lyrkan,

i apologize for the delay, right now, not the only project in queue. I haven't forgot this issue though.

weaverryan · 2017-09-14T19:38:26Z

I gotcha :). I'm am quite interested in this issue - so looking forward to it!

davidmpaz · 2017-11-15T20:58:14Z

Hi @Lyrkan , @weaverryan,

sorry the delay for this. I just updated the PR. After thinking on this I agree with @Lyrkan in which the tool is to general/powerful to limit it to only few use cases. I only leave path parameters as suggested.

I will write some more extended documentation after merge this, to at least have in some place the scenarios i did try as snippets or alike for other to use if needed.

As I mention at the beginning, this PR will fail because of the nsp check it already reported but not got too much attention I guess on the browsersync side.

cheers

ghost · 2018-11-22T05:22:58Z

I know this is an old issue but is this something Encore is continuing to want to achieve? I am looking at taking a bite into better HMR support and webpack-dev-server is a much better solution as far as development goes

davidmpaz · 2018-11-22T07:36:01Z

Hi @ProtonScott from my side it is fine. I will be happy if something better comes into play :-)

janmyszkier · 2019-08-26T16:24:46Z

@davidmpaz would you be so kind to rebase this PR ?

Use `plugin` key from object pushed as plugin into plugin array.

Bumped browser-sync depdendencies.

Upgraded all dependecies (yarn upgrade)

davidmpaz · 2019-09-20T14:56:22Z

Hi @janmyszkier,

here goes the rebase. I am not sure whether this is still relevant. The webpack plugin for browser-sync has not been updated in about 2 years (not sure what does that means) and i my self haven't been using encore/webpack in a while. So now i am a bit out of context.

I would love to hear if something better exist already out there. Or otherwise this is the something better now and it gets merged in the main stream :D

Best Regards,
David

janmyszkier · 2019-09-21T23:17:31Z

@Lyrkan is this something you can merge?

Lyrkan

Thank you for updating the PR @davidmpaz, I have some more remarks, probably caused by all the changes that happened in Encore since the last update.

That being said, before continuing working on it, I think we should check if all of that will be compatible with Webpack 5 (which should be coming "soon"). As you said the plugin hasn't been updated in a while and I'm afraid it will stop working and/or block us when we need to update Encore (I know that's already the case for some loaders/plugins we use...).

Lyrkan · 2019-09-23T13:10:54Z

index.js

+     *
+     * https://www.browsersync.io/
+     *
+     * Call it to start a proxy redirecting requests to `backendUrl` while


You're talking about backendUrl here and a bit below but no actual option is called like that, so it's a bit confusing. Did you mean proxy?

Lyrkan · 2019-09-23T13:15:00Z

lib/WebpackConfig.js

@@ -173,6 +173,11 @@ class WebpackConfig {
        this.terserPluginOptionsCallback = () => {};
        this.optimizeCssPluginOptionsCallback = () => {};
        this.notifierPluginOptionsCallback = () => {};
+        this.useImagesLoader = true;
+        this.useFontsLoader = true;


This line and the one above it already exist at the beginning of the constructor.

More generally, the settings have been divided into categories (flags, options, callback), so you'll also have to move useBrowserSync, browserSyncOptionsCallback and browserSyncOptions a bit.

Lyrkan · 2019-09-23T13:17:55Z

lib/WebpackConfig.js

@@ -708,6 +713,17 @@ class WebpackConfig {
        this.handlebarsConfigurationCallback = callback;
    }

+    enableBrowserSync(paths, browserSyncOptionsCallback = () => {}) {
+        this.useBrowserSync = true;
+        this.browserSyncOptions.paths = paths;


Should we also check if paths is an Array here? We could also allow string and put it into an array to allow enabling BrowserSync on a single path.

Lyrkan · 2019-09-23T13:23:04Z

lib/features.js

+        packages: [
+            { name: 'browser-sync' },
+            { name: 'browser-sync-webpack-plugin' },
+            { name: 'url' }


Is that package really needed? I don't see it being used anywhere else.

Lyrkan · 2019-09-23T13:24:55Z

package.json

@@ -30,6 +30,8 @@
    "@babel/preset-env": "^7.4.0",
    "assets-webpack-plugin": "^3.9.7",
    "babel-loader": "^8.0.0",
+    "browser-sync": "^2.18.13",
+    "browser-sync-webpack-plugin": "^2.2.2",


browser-sync and browser-sync-webpack-plugin should only be declared as devDependencies.

👍 browser-sync is a package aimed for development. not a big issue to fix this, too

janmyszkier · 2019-10-01T18:07:06Z

@davidmpaz friendly bump :)

davidmpaz · 2019-10-02T04:42:03Z

Hi,

since new version of webpack is a risk factor for this I wanted to wait for the answer i made to the plugins developers. I by myself right now can not tell if there will be some breaks.

Maybe you can also make a friendly bump to them too :)

I will try to get time for addressing the comments from above. But honestly dont want to take my time neither the time of @Lyrcan on something that at end will not be use because of deprecation.

janmyszkier · 2019-10-02T06:01:20Z

IMO we shouldn't worry about something that's NOT YET there. If this gets removed, an issue can be made to make it compatible WHEN IT'S NOT. we have semver in the package dependencies for a reason.

Lyrkan · 2019-10-05T01:18:14Z

@janmyszkier I agree with the semver thing, but I'm not sure that adding a feature knowing that we may have to remove it a few versions later (causing a BC break) before maybe adding it back again when it's fixed is a good thing for users (especially when it's something that they can already do quite easily with Encore.addPlugin())...

But if you want to check if the plugin works fine with Webpack 5 I started to work on the migration in #645. If it does I don't have any objection to merging this PR (apart from my few comments above).

davidmpaz · 2019-10-07T05:58:44Z

Hi @Lyrkan,

thanks for mentioning you are doing some work already in direction Webpack 5 compatibility check, that for sure will provide useful information. You mention before, this functionality could be added via Encore.addPlugin(), i would need to check the feature because i am not familiar with it or at least don't remember to have used it.

My take with this one would be at first decide whether this feature can be covered completely with the Encore.addPlugin() and decide whether we add support for this, or rely on projects adding this as needed later on via Encore.addPlugin().

Reason? Encore is already an sugar syntax of Webpack configuration, it is good to have support out of the box with checks and all for some features, still if this introduce risky libraries or APIs i would discard it completely in favor of having a clean and stable internal API.

I need help and advice on deciding this, i am open to suggestions. I did implement this 2 years ago, this means i will be fine discarding it because the whole thing got deprecated or outdated, it is normal when some code like this one hang around for so long. If we decide to continue to adding support for this, i will gladly take it further after the compatibility check work shed some light on the effectiveness on moving forward.

Make this sense to you @janmyszkier?

/cc @weaverryan

Regards

janmyszkier · 2020-07-12T21:46:34Z

@davidmpaz it does. thank you for the answer. I don't consider myself an expert on webpack nor on what both projects are planning, but if there's anything I can investigate to help out, let me know. Otherwise all I can do is cross my fingers for solving this.

versedi · 2020-07-29T20:27:16Z

@davidmpaz it does. thank you for the answer. I don't consider myself an expert on webpack nor on what both projects are planning, but if there's anything I can investigate to help out, let me know. Otherwise all I can do is cross my fingers for solving this.

@janmyszkier Just a question, why is the Browser Sync so valuable if there's a hot auto-reload working out-of-the-box already with any browsers without a need for addon?

Isn't that kind of replicating the feature just for sake of supporting another tool that does the same job? Are there any pros to using browsersync over normal dev-server that I'm not aware off?

janmyszkier · 2020-07-30T19:46:24Z

@versedi
because browsersync copies your moves between the devices and allows you for WAY EASIER design and behaviour testing on multiple devices.

scroll down on one device, page scrolls down on all connected devices
visit some url via click, page is visited on all connected devices
that's what auto reload doesn't do

aarongerig · 2020-07-31T08:34:29Z

@janmyszkier Meanwhile you can use this https://responsively.app/, which basically does the same as Browsersync. It's a great alternative.

davidmpaz mentioned this pull request Aug 23, 2017

nsp check --output summary vulnerabilities found BrowserSync/browser-sync#1416

Closed

11 tasks

Lyrkan reviewed Aug 23, 2017

View reviewed changes

davidmpaz mentioned this pull request Mar 6, 2018

Add BrowserSync support #2

Open

davidmpaz added 13 commits September 18, 2019 21:57

Add browser-sync dependencies

a6c13a3

Add feature requirements for browser sync

18cdc59

Add public api for browser sync integration

be76aee

Add internal module to integrate browser sync plugin

6e46049

Add feature packages to check against to

30c8298

Add plugin tests

7c372cc

Add tests for public api

126fefc

Add tests to config generator

cfdfe95

Fix tests to take into account plugin object

f7e9514

Use `plugin` key from object pushed as plugin into plugin array.

Remove backendUrl from tests

4203c04

Make linter happy with spaces usage

52ee7bb

Updated dependencies

34c4c2f

Bumped browser-sync depdendencies.

Bumped all dependencies

2cbc8b0

Upgraded all dependecies (yarn upgrade)

Lyrkan requested changes Sep 23, 2019

View reviewed changes

davidmpaz mentioned this pull request Sep 23, 2019

Webpack 5 compatibility Va1/browser-sync-webpack-plugin#83

Closed

This comment has been minimized.

Sign in to view

weaverryan changed the base branch from master to main November 18, 2020 18:21

This pull request was closed.

Browser sync #145

Browser sync #145

Conversation

davidmpaz commented Aug 23, 2017

Lyrkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidmpaz commented Aug 25, 2017

weaverryan commented Sep 13, 2017

davidmpaz commented Sep 13, 2017

weaverryan commented Sep 14, 2017

davidmpaz commented Nov 15, 2017

ghost commented Nov 22, 2018

davidmpaz commented Nov 22, 2018

janmyszkier commented Aug 26, 2019

davidmpaz commented Sep 20, 2019

janmyszkier commented Sep 21, 2019

Lyrkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janmyszkier commented Oct 1, 2019

davidmpaz commented Oct 2, 2019

janmyszkier commented Oct 2, 2019

Lyrkan commented Oct 5, 2019

davidmpaz commented Oct 7, 2019

This comment has been minimized.

janmyszkier commented Jul 12, 2020

versedi commented Jul 29, 2020 • edited Loading

janmyszkier commented Jul 30, 2020 • edited Loading

aarongerig commented Jul 31, 2020

versedi commented Jul 29, 2020 •

edited

Loading

janmyszkier commented Jul 30, 2020 •

edited

Loading