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

Relaxed public path requirements with dev-server #96

Merged

Conversation

weaverryan
Copy link
Member

Fixes #59 and finishes #66.

  • Adds a new --keep-public-path option for dev-server. When used, your publicPath is not prefixed with the dev server URL. For dev-server inside docker? #59, this means you can use setPublicPath('/build') with the dev-server, and your assets will remain local (i.e. /build/main.js instead of http://localhost:8080/build/main.js).

  • It is now possible to pass an absolute URL to setPublicPath() without an error when using dev-server. But, we issue a big warning, because this means your assets will point to to that absolute URL, instead of to the dev-server (which for most setups, is not what you want).

@samjarrett I'd love to confirm that this would solve your issue in Docker :).

@samjarrett
Copy link

samjarrett commented Jul 16, 2017

Hey @weaverryan thanks for the heads-up! I gave this a go with config like this:

if (!Encore.isProduction()) {
    Encore
        .setPublicPath('http://dev.vcap.me:8080')
        .setManifestKeyPrefix('/assets/build/')
    ;
}

and when I run the command with encore dev-server --host 0.0.0.0, I get the warning on the command line about using setPublicPath() and dev-server, but it seems like the config was being ignored?

Example - paths to CSS are being generated as relative:

<link href="/assets/build/global.css" rel="stylesheet">

if I disable the above Encore settings in my webpack conf, my path to CSS is this:

<link href="http://0.0.0.0:8080/assets/build/global.css" rel="stylesheet">

have I missed something?

@weaverryan
Copy link
Member Author

Thanks @samjarrett! Hmm... I bet I missed one small detail - i'll take a look at this soon and ping back when I'm sure it's ready :)

@davidmpaz
Copy link
Contributor

Hi @weaverryan,
just a question, will this PR also address #78 ? ... from:

When used, your publicPath is not prefixed with the dev server URL

I think this will create some troubles when using live reloading with Browser Sync. Resources are not written in file system but kept in memory.

So do we need that url: http://localhost:8080 as prefix when developing or do we need to set it as absolute url in order to get latest updates from compilations?

How do you think i could do with this PR to address #78 ?

@weaverryan
Copy link
Member Author

Yo @davidmpaz!

will this PR also address #78

It won't, but this was step 1 towards that (they're kind of unrelated, but work in the same area). So, we're definitely going to address that :).

[weaverryan] When used, your publicPath is not prefixed with the dev server URL

[davidmpaz] I think this will create some troubles when using live reloading with Browser Sync. Resources are not written in file system but kept in memory.

You're 100% right. Your link/script tags need to be prefixed with the absolute URL to the dev-server... and, internally, webpack's publicPath needs to also be set to the absolute URL to the dev-server (+ the output directory, so something like http://localhost:8080/build). The latter is needed for code splitting, and believe it's used in the compiled CSS files to point to assets like fonts, images, etc.

Buuuuut, in @samjarrett's situation, the dev-server is setup to bind at 0.0.0.0:8080, but from the perspective of his app, the dev-server is running at http://dev.vcap.me:8080. In other words, there's a disconnect between the host that we tell the dev-server to bind to... and the host that the app should actually use for fetch assets.

tl;dr; for 99% of the use-cases, the assets will still be prefixed with the dev server URL. But this PR allows them to be disconnected. Well, it will once I get it fully working ;).

How do you think i could do with this PR to address #78 ?

I just shot you a question over on that issue. When we answer that, I think it'll be a pretty easy PR. We're throwing the exception now because we simply cannot determine the contentBase in your situation.

@davidmpaz
Copy link
Contributor

@weaverryan, got it thanks!!

robertfausk and others added 7 commits July 20, 2017 20:34
when using dev-server and an absolute URL for publicPath
to get webpack encore working in docker environment
test for successful config set when using devServer and setPublicPath
with an absolute URL
- when using devServer and absolute URL for publicPath
- also add test case
@weaverryan weaverryan force-pushed the relaxed-public-path-requirements branch from 9848b4f to 4bc1e19 Compare July 21, 2017 00:50
@weaverryan
Copy link
Member Author

My apologies @samjarrett - this is (and was) totally ready to go after all :). There's one mistake in your script:

if (!Encore.isProduction()) {
    Encore
        .setPublicPath('http://dev.vcap.me:8080')
        // I removed the opening slash
        .setManifestKeyPrefix('assets/build/')
    ;
}

The manifest key normally doesn't start with a / - we make sure of that. So, I'm guessing (assuming you're using Symfony) that your template looks like this:

<script src="{{ asset('assets/build/main.js') }}">

Because you had .setManifestKeyPrefix('/assets/build/'), it changed the keys in manifest.json to start with a /, and so the path (assets/build/main.js) wasn't found in manifest.json and wasn't transformed (i.e. it was left alone).

I'm going to make a PR to guarantee that setManifestKeyPrefix() has no opening /. I originally allowed this... to add a bit more flexibility. But it's going to mess people up I think.

Soooo, make this one line change, and let me know how things look. I appreciate it!

@samjarrett
Copy link

Works well @weaverryan, thanks!

@weaverryan weaverryan merged commit 4bc1e19 into symfony:master Jul 21, 2017
weaverryan added a commit that referenced this pull request Jul 21, 2017
…ausk, weaverryan)

This PR was merged into the master branch.

Discussion
----------

Relaxed public path requirements with dev-server

Fixes #59 and finishes #66.

* Adds a new `--keep-public-path` option for `dev-server`. When used, your `publicPath` is not prefixed with the dev server URL. For #59, this means you can use `setPublicPath('/build')` with the dev-server, and your assets will remain local (i.e. `/build/main.js` instead of `http://localhost:8080/build/main.js`).

* It is now possible to pass an absolute URL to `setPublicPath()` without an error when using `dev-server`. But, we issue a big warning, because this means your assets will point to to that absolute URL, instead of to the dev-server (which for most setups, is not what you want).

@samjarrett I'd love to confirm that this would solve your issue in Docker :).

Commits
-------

4bc1e19 Using real public path, though it doesn't look like it matters
92e22af Allowing an absolute publicPath with dev-server, but showing a warning
830fdb5 Adding --keep-public-path to dev-server to allow you to fully control the publicPath
b27f7c9 Reversing some of the changes we won't do for now, and adding the failing test
e206a12 fix issue in generated public path of manifest.json
eb5565b convert error into warning
910b6bc convert error into warning
@weaverryan weaverryan deleted the relaxed-public-path-requirements branch July 21, 2017 21:21
@weaverryan
Copy link
Member Author

Cheers - thanks for checking @samjarrett!

@adrienbrault
Copy link

@weaverryan I can't figure out how to use this feature, I keep getting Unknown argument: keep-public-path, see the output below:

# ./node_modules/.bin/encore dev-server --keep-public-path --disable-host-check --port 8204 --host 0.0.0.0
Running webpack-dev-server ...

webpack-dev-server 2.9.1
webpack 3.6.0
Usage: https://webpack.js.org/configuration/dev-server/

Config options:
  --config       Path to the config file
                         [string] [default: webpack.config.js or webpackfile.js]
...
  --open-page    Open default browser with the specified page           [string]

Unknown argument: keep-public-path

even though the keep-public-path option is shown in the --help:

# ./node_modules/.bin/encore dev-server --help
usage encore [dev|production|dev-server]

encore is a thin executable around the webpack or webpack-dev-server executables

Commands:
    dev        : runs webpack for development
       - Supports any webpack options (e.g. --watch)

    dev-server : runs webpack-dev-server
       - --host The hostname/ip address the webpack-dev-server will bind to
       - --port The port the webpack-dev-server will bind to
       - --hot  Enable HMR on webpack-dev-server
       - --keep-public-path Do not change the public path (it is usually prefixed by the dev server URL)
       - Supports any webpack-dev-server options

    production : runs webpack for production
       - Supports any webpack options (e.g. --watch)

    encore dev --watch
    encore dev-server
    encore production

Running webpack-dev-server ...

webpack-dev-server 2.9.1
webpack 3.6.0
Usage: https://webpack.js.org/configuration/dev-server/

Config options:
...

@Lyrkan
Copy link
Collaborator

Lyrkan commented Oct 4, 2017

@adrienbrault It's caused by the fact that the --keep-public-path argument is supposed to be handled by Encore only but webpack-dev-server picks it up and doesn't recognize it. Based on webpack/webpack#2254 custom arguments need to be prefixed by env. (e.g. env.keep-public-path).

Unless I'm missing something too it didn't seem to work in the version it was released in either (v0.12.0), would you mind opening an issue about it ?

@weaverryan
Copy link
Member Author

@Lyrkan Are you saying that the --keep-public-path is broken everywhere? This definitely worked when I merged - I was testing it directly. But, @adrienbrault's output look like a bug.

@Lyrkan
Copy link
Collaborator

Lyrkan commented Oct 6, 2017

@weaverryan Looks like it...

$ mkdir encore-public-path && cd encore-public-path

$ yarn add --dev @symfony/webpack-encore@0.12.0
yarn add v1.0.2
info No lockfile found.
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
success Saved lockfile.
success Saved 603 new dependencies.
├─ @symfony/webpack-encore@0.12.0
(...)
Done in 62.17s.

$ ./node_modules/.bin/encore dev-server --keep-public-path
Running webpack-dev-server ...

webpack-dev-server 2.9.1
webpack 3.6.0
Usage: https://webpack.js.org/configuration/dev-server/

Config options:
(...)
Unknown argument: keep-public-path

I don't know what could've caused it to break... maybe a dependency that got updated? The issue that I referenced in my previous comment seems quite old though.

@pscheit
Copy link

pscheit commented Feb 22, 2018

glad I found this thread.
@weaverryan you said it's and edge case and for 95% this should work. Well maybe when 5% only use docker. Because this is what happens:

When I run the wepack-dev-server in the docker container, it should not bind to localhost (and would be inaccessible from docker-host) We HAVE to use --host 0.0.0.0, so that the manifest.json will always contain: http:/0.0.0.0/assets/build/something.js. But this makes no sense in any scenario, right?

Shouldn't setting the publicPath be mentioned here:
https://symfony.com/doc/current/frontend/encore/dev-server.html

best regards
philipp

@weaverryan
Copy link
Member Author

Yea - it almost definitely should. And we should mention Docker specifically. Could you open a doc PR or issue and dump your information there? It would be really helpful :)

@pscheit
Copy link

pscheit commented Feb 22, 2018

i will

pscheit added a commit to pscheit/symfony-docs that referenced this pull request Feb 26, 2018
… to all interfaces

This is the discussion, for setting an absolute url as publicPath, when you're binding the dev server not to localhost but to any external interface (0.0.0.0) and disable host checking.

This is the related discussion:
symfony/webpack-encore#96

best regards
philipp
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.

dev-server inside docker?
7 participants