-
Notifications
You must be signed in to change notification settings - Fork 809
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
BrowserSync Error on fresh installation #850
Comments
Heya, thanks for your issue. The file is part of your own app so you can change it to whatever you want. Welcoming PR's to improve it further. |
Hey @driesvints , thanks for getting back so quickly. And well, I can agree to that: I'm able to change it to whatever I want and I already did that - so it's working fine on my end now. The reason why I reported the issue is that the default file that is being generated by Jetstream using the app.blade.php stub for Inertia is throwing an error, which could hold off new Laravel users from getting into the Laravel world in the first place, as they would get an error by default. So my impression was, that the PR mentioned above made something to be the default, that leads to errors after a fresh installation and that actually could be something I can easily add whenever I need it (Laravel Mix docs have a section on this). But well, as I said, I didn't come up with a PR because I don't know which behavior you want to have here. |
Hey @paulmax-os. I think "Add browserSync to webpack.mix.js" is a good solution here. But you'd need to try a PR to see if Taylor would accept it. Thanks. |
This probably should stay open. There is no point a starter kit to come with a broken functionality. It is a trivial fix but inexperienced developers may be wondering why their frontend app is stalling for a few seconds before timing out. |
We're still open to PR's. |
Hey everyone,
this is the first issue I'm submitting, so if I'm doing anything wrong, please let me know - I am more than happy to learn and adapt.
Description:
Using the regular laravel installer to setup a fresh version of laravel or Sail an error will be thrown on the client when opening up the app (not tested with Valet):
I believe this was introduced by PR: #797 , where the import of the script
https://localhost:3000/browser-sync/browser-sync-client.js
was hardcoded for whenever the app is running locally into app.blade.php, meaning that the client will try to load this script in local env even if there is no sync server - and having no sync server running seems to be the default even if you go withnpm run watch
.Another thing that could be problematic with hardcoding the URL of the browser-sync-client.js : It will also fail if there already is an application that is running on this port, which shouldn't be that unusual considering the port number.
I don't know if this actually has great impact to an end user as the issue can be fixed somewhat quickly. But I can imagine that it could lead to some negative emotional reaction, stemming from the situation that you're doing a fresh install and the first thing you're seeing is an error popping up in your console. For newcomers this might be frustrating.
Possible Solutions I can think of at the moment
I am not really sure about the behavior that is actually desired. That's why I didn't open up a PR, but I am happy to help, working on a solution.
Add browserSync to webpack.mix.js
One could add a few lines to webpack.mix.js, making sure there is a browserSync in non production environments:
This works when running
npm run watch
but not withnpm run dev
.Revert #797 and add it to the docs
One could also remove the lines added by #797 and add a section to the docs, explaining how to resolve the issue for Valet.
Keep #797 and add step to installation docs
One could also keep the lines and add a section to the installation of Jetstream Inertia, that explain what is happening with browserSync, how to get it to work and why it won't work on
npm run dev
.Steps To Reproduce:
laravel new example-app -jet
or create a fresh laravel app using sail and install Jetstream Inertia.The text was updated successfully, but these errors were encountered: