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

LaravelMix example: process.env.DDEV_HOSTNAME only works without additional project hostnames #27

Closed
mandrasch opened this issue Oct 10, 2022 · 14 comments

Comments

@mandrasch
Copy link

mandrasch commented Oct 10, 2022

Hey, just as information, thanks for providing this:

Laravel Mix example (https://github.com/drud/ddev-browsersync#laravel-mix-configuration) suggests using

let url = process.env.DDEV_HOSTNAME;

mix.js('resources/js/app.js', 'public/js')
    .postCss('resources/css/app.css', 'public/css', [
        //
    ])
    .browserSync({
        proxy: "localhost",
        host:  url,
        open:  false,
        ui: false
    });

When there are Additional Hostnames configured in DDEV, DDEV_HOSTNAME returns a comma separated list.

For example main-site.ddev.site,subsite-1.ddev.site,subsite-2.ddev.site

We also noticed that host can be disabled all together and integration of browsersync still works? (I guess because proxy is used anyway and host is only for cli terminal output). So maybe we could leave it out of the example to avoid confusion? Not quite sure yet. 🤔

mix.js('resources/js/app.js', 'public/js')
    .postCss('resources/css/app.css', 'public/css', [
        //
    ])
    .browserSync({
        proxy: "localhost",
        open:  false,
        ui: false
    });

https://browsersync.io/docs/options#option-host

Best regards,
Matthias
PS: I could provide a sample repo if needed for reproduction.

@rfay
Copy link
Member

rfay commented Oct 10, 2022

A basic idea to make sure to use a single hostname would be ${DDEV_PROJECT}.${DDEV_TLD}

@tyler36
Copy link
Collaborator

tyler36 commented Oct 11, 2022

Thanks @mandrasch .

Any interest in a PR?

@mandrasch
Copy link
Author

Hey @tyler36, sure thing, currently occupied with other things. Hope I'll get to it in the next days! Cheers.

@mandrasch
Copy link
Author

mandrasch commented Nov 10, 2022

Hey, I prepared a demo repo with Randys suggestion: https://github.com/mandrasch/ddev-laravelmix-browsersync

For me it works fine using

let url = `${process.env.DDEV_PROJECT}.${process.env.DDEV_TLD}`;

But as far as I understand this is only for command line output. And "http" is displayed instead of "https".

image

Maybe we should just leave it out to avoid confusion?

Without .host it also works and output is the following:

mix.browserSync({
    proxy: "localhost",
    open: false,
    ui: false
});

image

@rfay
Copy link
Member

rfay commented Nov 10, 2022

Side note: additional_hostnames support (and additional_fqdns) added to ddev-varnish in ddev/ddev-varnish#9, not sure if this is a related situation.

@mandrasch
Copy link
Author

Thanks for info! Haven't used varnish yet, can't say if this is related.

Regarding LaravelMix:

Added additional hostnames to my demo, works fine as well because it justs responds to :3000 port anyway.

LaravelMix has no support for subdomains / additional hostnames as far as I know, it's just compiles files based on one config and outputs them to the output folder.

My educated guess would be that we could just strip out the .host-setting 🤔 But maybe @tyler36 knows more about use cases.

@mandrasch
Copy link
Author

Docs:

@tyler36
Copy link
Collaborator

tyler36 commented Nov 11, 2022

@mandrasch

I was able to replicate the issue you describe. It occures with the laravel-mix and the vanilla javascript included.

I did a PR which explicity sets the URL (as suggested by @rfay ) and includes a docs update on additional hostnames. I prefer to see a project-based URL instead of an IP-based URL, which would be the case if we dropped the host config option.

How common are additional hostname projects? I think the PR works but it breaks auto-updates so perhaps a better solution is required.

@rfay
Copy link
Member

rfay commented Nov 11, 2022

There are quite a lot of uses for additional_hostnames and additional_fqdns.

@tyler36
Copy link
Collaborator

tyler36 commented Nov 14, 2022

Change the PR to support addition hostnames out of the box.

@mandrasch does that work for you?

@mandrasch
Copy link
Author

Change the PR to support addition hostnames out of the box.

@mandrasch does that work for you?

Hey sorry for the delay, can I install your updated PR as well with ddev get https://github.com/tyler36/ddev-browsersync/tarball/tyler36/https-support/ to test it?

(Test repo https://github.com/mandrasch/ddev-laravelmix-browsersync)

@mandrasch
Copy link
Author

Just tried, doesn't work anymore: Unable to download https://github.com/tyler36/ddev-browsersync/tarball/tyler36/https-support/: Unable to download https://github.com/tyler36/ddev-browsersync/tarball/tyler36/https-support/: download link https://github.com/tyler36/ddev-browsersync/tarball/tyler36/https-support/ returned wrong status code: got 404 want 200

@tyler36
Copy link
Collaborator

tyler36 commented Nov 21, 2022

@mandrasch

I think the tarball for #21 is: ddev get https://github.com/drud/ddev-browsersync/tarball/tyler36/https-support

@mandrasch
Copy link
Author

mandrasch commented Nov 21, 2022

Thanks very much, I pulled latest version into https://github.com/mandrasch/ddev-laravelmix-browsersync, did not change much in my repo. Still worked.

For my personal use cases I'll go without setting the host since I see no benefit in setting this (and also think it is confusing to read 'http://my-ddev-site.ddev.site:3000' instead of 'https://...:3000').

mix.browserSync({
    proxy: "localhost",
    open: false,
    ui: false
});

Thanks very much for the discussion here! Much appreciated! 🙏 🙏

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

No branches or pull requests

3 participants