Skip to content
This repository was archived by the owner on Mar 31, 2023. It is now read-only.

Updating browsersync config “domain” to use proxy and host #87

Open
danny-englander wants to merge 1 commit intofourkitchens:developfrom
danny-englander:develop
Open

Updating browsersync config “domain” to use proxy and host #87
danny-englander wants to merge 1 commit intofourkitchens:developfrom
danny-englander:develop

Conversation

@danny-englander
Copy link

Even though config.browserSync.domain, was mapped to proxy, It might be confusing to use "domain" as a local browserSync setting as there is actually a real setting for domain in browserSync. I also discovered that it works best if host is used in tandem with proxy.

However, this could be an edge case. I'm using Docksal so it was crucial for host and proxy to be used together for browserSync to work on the Docksal domain. Note the browserSync setting for "domain" (AKA socket) actually has to do with using in tandem for socket.io. https://browsersync.io/docs/options#option-socket

I also added in some more standard options that are nice to have within the proxy if statement which I also updated to check for the two conditions needed.

If this pull request is not acceptable, I'm fine with just running a patch automatically after npm install.

amazingrando
amazingrando previously approved these changes Nov 21, 2018
Copy link
Contributor

@evanmwillhite evanmwillhite left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @danny-englander, but I just noticed a couple of things (besides merge conflicts) - one related to the PR and one not that make me hesitant to merge as-is.

Related to PR:
If proxy and/or host can be used, we should make it possible to just use one or the other which means at the very least replacing the and with an or on line 99 of index.js (and the console message after it).

Unrelated to PR:
When using this feature (which entailed setting my baseDir and startPath accordingly), I was able to get the url to work but a lot of things linked in Pattern Lab were then broken, the most troubling of which were the JavaScript files that we attach via our custom attach_library Drupal Twig extension.

I guess on the 2nd we could open up a separate PR in the repo for that but should at least address the first item and the merge conflicts. @danny-englander you up for that? If not, do you mind creating a new PR on a non-forked branch (you have access to contribute to this repo)? I want you to get credit but someone else can't make commits to the forked branch. Thank you again so much for your work and support of Emulsify!!

@danny-englander
Copy link
Author

@evanmwillhite Thanks for all the detailed information! I don't have a lot of free time right now to go back and work on this so I am not sure when I would have time at the moment.

@evanmwillhite
Copy link
Contributor

@danny-englander That's OK, thanks for responding. If someone has time on our end, we can just open up a separate PR. Thanks again!

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.

3 participants