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

Dont hide output from browsersync command #38

Merged
merged 2 commits into from
Jan 20, 2023

Conversation

mmunz
Copy link
Contributor

@mmunz mmunz commented Jan 18, 2023

Is there any reason not to show the output from browser-sync?
This makes it more difficult to debug, e.g. see when files are reloaded.

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO what we should do is pipe it to grep to get rid of the confusing output but keep the update notifications. IIRC this was introduced in #31 and it was introduced because people were confused by the info given about the port (telling people to hit localhost:3000)

@mmunz
Copy link
Contributor Author

mmunz commented Jan 18, 2023

@rfay i have added a grep command to remove those misleading urls

@tyler36 tyler36 requested a review from rfay January 19, 2023 00:55
@tyler36
Copy link
Collaborator

tyler36 commented Jan 19, 2023

Looks good to me.

@rfay
Copy link
Member

rfay commented Jan 19, 2023

Can be tested with ddev get https://github.com/mmunz/ddev-browsersync/tarball/dont-hide-browsersync-output

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem to work as expected, still shows the bogus URLs:

$ ddev browsersync
[Browsersync] Proxying: http://localhost
[Browsersync] Access URLs:
 ------------------------------------
    Local: http://localhost:3000
 External: http://d9.example.com:3000
 ------------------------------------
[Browsersync] Watching files...

Also, I note that the README still says

NOTE: The browsersync'd URL is HTTPS, not HTTP. ddev-router redirects traffic to HTTPS, but browsersync does not know this.

@tyler36
Copy link
Collaborator

tyler36 commented Jan 20, 2023

@rfay I'm not seeing that.

$ ddev browsersync
Proxying browsersync on https://browsersync-demo.ddev.site:3000
[Browsersync] Proxying: http://localhost
[Browsersync] Watching files...

When I remove the | grep -v "Access URLs\|--------------------\|Local: http\|External: http"

$ ddev browsersync
Proxying browsersync on https://browsersync-demo.ddev.site:3000
[Browsersync] Proxying: http://localhost
[Browsersync] Access URLs:
 ------------------------------------------------
    Local: http://localhost:3000
 External: http://browsersync-demo.ddev.site:3000
 ------------------------------------------------
[Browsersync] Watching files...

@tyler36
Copy link
Collaborator

tyler36 commented Jan 20, 2023

My complete browsersync file. I added TESTING to the first echo to confirm the correct file is used.

#!/bin/bash

#ddev-generated
## Description: Run browser-sync in the web container
## Usage: browsersync
## Example: "ddev browsersync"
## ExecRaw: true

echo "Proxying browsersync on ${DDEV_PRIMARY_URL}:3000 TESTING"
browser-sync start -c /var/www/html/.ddev/browser-sync.js | grep -v "Access URLs\|--------------------\|Local: http\|External: http"
ddev browsersync
Proxying browsersync on https://browsersync-demo.ddev.site:3000 TESTING
[Browsersync] Proxying: http://localhost
[Browsersync] Watching files...

DDEV: 1.21.4
OS: Ubuntu

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had an old browsersync global command that didn't have #ddev-generated in it, so didn't get overwritten properly on ddev get.

Now I see the expected results:

$ ddev browsersync
Proxying browsersync on https://d9.example.com:3000
[Browsersync] Proxying: http://localhost
[Browsersync] Watching files...
[Browsersync] Reloading Browsers...
[Browsersync] File event [change] : web/core/profiles/demo_umami/themes/umami/css/base.css

I kind of wonder if this custom command should be installed at the project level, and I'll take a look and see if the ddev get warned me that it was skipping it.

@rfay
Copy link
Member

rfay commented Jan 20, 2023

It did in fact warn me, but I wasn't looking, like most people wouldn't be.

NOT overwriting file/directory /Users/rfay/.ddev/commands/web/browsersync. The #ddev-generated signature was not found in the file, so it will not be overwritten. You can just remove the file and use ddev get again if you want it to be replaced: signature was not found in file /Users/rfay/.ddev/commands/web/browsersync

@tyler36 tyler36 merged commit 7feef02 into ddev:main Jan 20, 2023
@mmunz mmunz deleted the dont-hide-browsersync-output branch January 20, 2023 06:22
@mmunz
Copy link
Contributor Author

mmunz commented Jan 20, 2023

I kind of wonder if this custom command should be installed at the project level, and I'll take a look and see if the ddev get warned me that it was skipping it.

Also a good point, i was wondering about this too already. The globally installed command shows up in other ddev projects where the browser-sync addon is not installed. But it does not work there of course.

@mmunz
Copy link
Contributor Author

mmunz commented Jan 20, 2023

Thanks for the fast review and merging.

@tyler36
Copy link
Collaborator

tyler36 commented Jan 20, 2023

I kind of wonder if this custom command should be installed at the project level, and I'll take a look and see if the ddev get warned me that it was skipping it.

Hope to get around to making a PR to move it to a project-level command.

@tyler36
Copy link
Collaborator

tyler36 commented Jan 20, 2023

@mmunz Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants