Browser sync #145
Browser sync #145
Conversation
index.js
Outdated
| * @param {function} browserSyncOptionsCallback | ||
| * @return {publicApi} | ||
| */ | ||
| enableBrowserSync(backendUrl, paths, browserSyncOptionsCallback = () => {}) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- 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;
});
lib/plugins/browser-sync.js
Outdated
| let serverParts = url.parse(webpackConfig.browserSyncOptions.backendUrl, true); | ||
| let browserSyncConfig = Object.assign({ | ||
| proxy: webpackConfig.browserSyncOptions.backendUrl, | ||
| port: serverParts.port || '80', |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.).
lib/plugins/browser-sync.js
Outdated
| loaderFeatures.ensurePackagesExist('browsersync'); | ||
| const BrowserSyncPlugin = require('browser-sync-webpack-plugin'); | ||
|
|
||
| let browserSyncOptions = {}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
you are right. I will move initialization there.
Regarding let or const here, the object is changed in callbacks, would not this bring troubles ?
There was a problem hiding this comment.
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 }
lib/plugins/browser-sync.js
Outdated
| const BrowserSyncPlugin = require('browser-sync-webpack-plugin'); | ||
|
|
||
| let browserSyncOptions = {}; | ||
| let browserSyncPluginOptions = {}; |
There was a problem hiding this comment.
Same remarks than for the browserSyncOptions variable.
lib/plugins/browser-sync.js
Outdated
| ] | ||
| }, browserSyncOptions); | ||
|
|
||
| let browserSyncPluginConfig = Object.assign({ |
|
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 |
|
Ping @davidmpaz! No rush - just wanted to remind you. I also just got back from several weeks away ;) |
|
Hey @weaverryan , @Lyrkan, i apologize for the delay, right now, not the only project in queue. I haven't forgot this issue though. |
|
I gotcha :). I'm am quite interested in this issue - so looking forward to it! |
|
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 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 cheers |
|
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 |
|
Hi @ProtonScott from my side it is fine. I will be happy if something better comes into play :-) |
|
@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)
|
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, |
|
@Lyrkan is this something you can merge? |
There was a problem hiding this comment.
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...).
| * | ||
| * https://www.browsersync.io/ | ||
| * | ||
| * Call it to start a proxy redirecting requests to `backendUrl` while |
There was a problem hiding this comment.
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?
| @@ -173,6 +173,11 @@ class WebpackConfig { | |||
| this.terserPluginOptionsCallback = () => {}; | |||
| this.optimizeCssPluginOptionsCallback = () => {}; | |||
| this.notifierPluginOptionsCallback = () => {}; | |||
| this.useImagesLoader = true; | |||
| this.useFontsLoader = true; | |||
There was a problem hiding this comment.
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.
| @@ -708,6 +713,17 @@ class WebpackConfig { | |||
| this.handlebarsConfigurationCallback = callback; | |||
| } | |||
|
|
|||
| enableBrowserSync(paths, browserSyncOptionsCallback = () => {}) { | |||
| this.useBrowserSync = true; | |||
| this.browserSyncOptions.paths = paths; | |||
There was a problem hiding this comment.
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.
| packages: [ | ||
| { name: 'browser-sync' }, | ||
| { name: 'browser-sync-webpack-plugin' }, | ||
| { name: 'url' } |
There was a problem hiding this comment.
Is that package really needed? I don't see it being used anywhere else.
| @@ -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", | |||
There was a problem hiding this comment.
browser-sync and browser-sync-webpack-plugin should only be declared as devDependencies.
There was a problem hiding this comment.
👍 browser-sync is a package aimed for development. not a big issue to fix this, too
|
@davidmpaz friendly bump :) |
|
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. |
|
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. |
|
@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 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). |
|
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 My take with this one would be at first decide whether this feature can be covered completely with the 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 |
This comment has been minimized.
This comment has been minimized.
|
@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? |
|
@versedi
|
|
@janmyszkier Meanwhile you can use this https://responsively.app/, which basically does the same as Browsersync. It's a great alternative. |
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