feature #96 Relaxed public path requirements with dev-server (robertf…

…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

bin/encore.js

@@ -10,7 +10,6 @@
 
 'use strict';
 
-const path = require('path');
 const parseRuntime = require('../lib/config/parse-runtime');
 const context = require('../lib/context');
 const chalk = require('chalk');
@@ -62,11 +61,14 @@ function showUsageInstructions() {
     console.log('Commands:');
     console.log(`    ${chalk.green('dev')}        : runs webpack for development`);
     console.log('       - Supports any webpack options (e.g. --watch)');
+    console.log();
     console.log(`    ${chalk.green('dev-server')} : runs webpack-dev-server`);
     console.log(`       - ${chalk.yellow('--host')} The hostname/ip address the webpack-dev-server will bind to`);
     console.log(`       - ${chalk.yellow('--port')} The port the webpack-dev-server will bind to`);
     console.log(`       - ${chalk.yellow('--hot')}  Enable HMR on webpack-dev-server`);
+    console.log(`       - ${chalk.yellow('--keep-public-path')} Do not change the public path (it is usually prefixed by the dev server URL)`);
     console.log('       - Supports any webpack-dev-server options');
+    console.log();
     console.log(`    ${chalk.green('production')} : runs webpack for production`);
     console.log('       - Supports any webpack options (e.g. --watch)');
     console.log();

index.js

@@ -14,13 +14,17 @@ const configGenerator = require('./lib/config-generator');
 const validator = require('./lib/config/validator');
 const PrettyError = require('pretty-error');
 const runtimeConfig = require('./lib/context').runtimeConfig;
+const logger = require('./lib/logger');
 
 // at this time, the encore executable should have set the runtimeConfig
 if (!runtimeConfig) {
     throw new Error('Are you trying to require index.js directly?');
 }
 
 let webpackConfig = new WebpackConfig(runtimeConfig);
+if (runtimeConfig.verbose) {
+    logger.verbose();
+}
 
 module.exports = {
     /**

lib/WebpackConfig.js

@@ -86,23 +86,11 @@ class WebpackConfig {
     }
 
     setPublicPath(publicPath) {
-        /*
-         * Do not allow absolute URLs *and* the webpackDevServer
-         * to be used at the same time. The webpackDevServer basically
-         * provides the publicPath (and so in those cases, publicPath)
-         * is simply used as the default manifestKeyPrefix.
-         */
-        if (publicPath.includes('://')) {
-            if (this.useDevServer()) {
-                throw new Error('You cannot pass an absolute URL to setPublicPath() and use the dev-server at the same time. Try using Encore.isProduction() to only configure your absolute publicPath for production.');
-            }
-        } else {
-            if (publicPath.indexOf('/') !== 0) {
-                // technically, not starting with "/" is legal, but not
-                // what you want in most cases. Let's not let the user make
-                // a mistake (and we can always change this later).
-                throw new Error('The value passed to setPublicPath() must start with "/" or be a full URL (http://...)');
-            }
+        if (publicPath.includes('://') === false && publicPath.indexOf('/') !== 0) {
+            // technically, not starting with "/" is legal, but not
+            // what you want in most cases. Let's not let the user make
+            // a mistake (and we can always change this later).
+            throw new Error('The value passed to setPublicPath() must start with "/" or be a full URL (http://...)');
         }
 
         // guarantee a single trailing slash
@@ -129,13 +117,20 @@ class WebpackConfig {
      * @returns {string}
      */
     getRealPublicPath() {
-        // if we're using webpack-dev-server, use it & add the publicPath
-        if (this.useDevServer()) {
-            // avoid 2 middle slashes
-            return this.runtimeConfig.devServerUrl.replace(/\/$/,'') + this.publicPath;
+        if (!this.useDevServer()) {
+            return this.publicPath;
+        }
+
+        if (this.runtimeConfig.devServerKeepPublicPath) {
+            return this.publicPath;
+        }
+
+        if (this.publicPath.includes('://')) {
+            return this.publicPath;
         }
 
-        return this.publicPath;
+        // if using dev-server, prefix the publicPath with the dev server URL
+        return this.runtimeConfig.devServerUrl.replace(/\/$/,'') + this.publicPath;
     }
 
     addEntry(name, src) {

lib/config-generator.js

@@ -381,11 +381,12 @@ class ConfigGenerator {
 
         return {
             contentBase: contentBase,
-            publicPath: this.webpackConfig.publicPath,
+            // this doesn't appear to be necessary, but here in case
+            publicPath: this.webpackConfig.getRealPublicPath(),
             // avoid CORS concerns trying to load things like fonts from the dev server
             headers: { 'Access-Control-Allow-Origin': '*' },
-            // required by FriendlyErrorsWebpackPlugin
             hot: this.webpackConfig.useHotModuleReplacementPlugin(),
+            // required by FriendlyErrorsWebpackPlugin
             quiet: true,
             compress: true,
             historyApiFallback: true,

lib/config/RuntimeConfig.js

@@ -19,11 +19,13 @@ class RuntimeConfig {
         this.useDevServer = null;
         this.devServerUrl = null;
         this.devServerHttps = null;
+        this.devServerKeepPublicPath = false;
         this.useHotModuleReplacement = null;
 
         this.babelRcFileExists = null;
 
         this.helpRequested = false;
+        this.verbose = false;
     }
 }
 

lib/config/parse-runtime.js

@@ -29,17 +29,22 @@ module.exports = function(argv, cwd) {
         case 'dev':
             runtimeConfig.isValidCommand = true;
             runtimeConfig.environment = 'dev';
+            runtimeConfig.verbose = true;
             break;
         case 'production':
             runtimeConfig.isValidCommand = true;
             runtimeConfig.environment = 'production';
+            runtimeConfig.verbose = false;
             break;
         case 'dev-server':
             runtimeConfig.isValidCommand = true;
             runtimeConfig.environment = 'dev';
+            runtimeConfig.verbose = true;
+
             runtimeConfig.useDevServer = true;
             runtimeConfig.devServerHttps = argv.https;
             runtimeConfig.useHotModuleReplacement = argv.hot || false;
+            runtimeConfig.devServerKeepPublicPath = argv.keepPublicPath || false;
 
             var host = argv.host ? argv.host : 'localhost';
             var port = argv.port ? argv.port : '8080';

lib/config/validator.js

@@ -10,6 +10,7 @@
 'use strict';
 
 const pathUtil = require('./path-util');
+const logger = require('./../logger');
 
 class Validator {
     /**
@@ -46,9 +47,24 @@ class Validator {
     }
 
     _validateDevServer() {
-        if (this.webpackConfig.useVersioning && this.webpackConfig.useDevServer()) {
+        if (!this.webpackConfig.useDevServer()) {
+            return;
+        }
+
+        if (this.webpackConfig.useVersioning) {
             throw new Error('Don\'t enable versioning with the dev-server. A good setting is Encore.enableVersioning(Encore.isProduction()).');
         }
+
+        /*
+         * An absolute publicPath is incompatible with webpackDevServer.
+         * This is because we want to *change* the publicPath to point
+         * to the webpackDevServer URL (e.g. http://localhost:8080/).
+         * There are some valid use-cases for not wanting this behavior
+         * (see #59), but we want to warn the user.
+         */
+        if (this.webpackConfig.publicPath.includes('://')) {
+            logger.warning(`Passing an absolute URL to setPublicPath() *and* using the dev-server can cause issues. Your assets will load from the publicPath (${this.webpackConfig.publicPath}) instead of from the dev server URL (${this.webpackConfig.runtimeConfig.devServerUrl}).`);
+        }
     }
 }
 

lib/logger.js

@@ -0,0 +1,59 @@
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+'use strict';
+
+const chalk = require('chalk');
+let isVerbose = false;
+let quiet = false;
+let messages = {
+    debug: [],
+    warning: [],
+};
+
+function log(message) {
+    if (quiet) {
+        return;
+    }
+
+    console.log(message);
+}
+
+module.exports = {
+    debug(message) {
+        messages.debug.push(message);
+
+        if (isVerbose) {
+            log(`${chalk.bgBlack.white(' DEBUG ')} ${message}`);
+        }
+    },
+
+    warning(message) {
+        messages.warning.push(message);
+
+        log(`${chalk.bgYellow.black(' WARNING ')} ${chalk.yellow(message)}`);
+    },
+
+    clearMessages() {
+        messages.debug = [];
+        messages.warning = [];
+    },
+
+    getMessages() {
+        return messages;
+    },
+
+    quiet() {
+        quiet = true;
+    },
+
+    verbose() {
+        isVerbose = true;
+    }
+};

test/WebpackConfig.js

@@ -104,14 +104,43 @@ describe('WebpackConfig object', () => {
                 config.setPublicPath('foo/');
             }).to.throw('The value passed to setPublicPath() must start with "/"');
         });
+    });
+
+    describe('getRealPublicPath', () => {
+        it('Returns normal with no dev server', () => {
+            const config = createConfig();
+            config.setPublicPath('/public');
+
+            expect(config.getRealPublicPath()).to.equal('/public/');
+        });
 
-        it('Setting to a URL when using devServer throws an error', () => {
+        it('Prefix when using devServer', () => {
             const config = createConfig();
             config.runtimeConfig.useDevServer = true;
+            config.runtimeConfig.devServerUrl = 'http://localhost:8080/';
+            config.setPublicPath('/public');
 
-            expect(() => {
-                config.setPublicPath('https://examplecdn.com');
-            }).to.throw('You cannot pass an absolute URL to setPublicPath() and use the dev-server');
+            expect(config.getRealPublicPath()).to.equal('http://localhost:8080/public/');
+        });
+
+        it('No prefix with devServer & devServerKeepPublicPath option', () => {
+            const config = createConfig();
+            config.runtimeConfig.useDevServer = true;
+            config.runtimeConfig.devServerUrl = 'http://localhost:8080/';
+            config.runtimeConfig.devServerKeepPublicPath = true;
+            config.setPublicPath('/public');
+
+            expect(config.getRealPublicPath()).to.equal('/public/');
+        });
+
+        it('devServer does not prefix if publicPath is absolute', () => {
+            const config = createConfig();
+            config.runtimeConfig.useDevServer = true;
+            config.runtimeConfig.devServerUrl = 'http://localhost:8080/';
+            config.setPublicPath('http://coolcdn.com/public');
+            config.setManifestKeyPrefix('/public/');
+
+            expect(config.getRealPublicPath()).to.equal('http://coolcdn.com/public/');
         });
     });
 

test/config/parse-runtime.js

@@ -69,6 +69,7 @@ describe('parse-runtime', () => {
         expect(config.useDevServer).to.be.true;
         expect(config.devServerUrl).to.equal('http://localhost:8080/');
         expect(config.useHotModuleReplacement).to.be.false;
+        expect(config.devServerKeepPublicPath).to.be.false;
     });
 
     it('dev-server command with options', () => {
@@ -114,4 +115,12 @@ describe('parse-runtime', () => {
         expect(config.useDevServer).to.be.true;
         expect(config.useHotModuleReplacement).to.be.true;
     });
+
+    it('dev-server command --keep-public-path', () => {
+        const testDir = createTestDirectory();
+        const config = parseArgv(createArgv(['dev-server', '--keep-public-path']), testDir);
+
+        expect(config.useDevServer).to.be.true;
+        expect(config.devServerKeepPublicPath).to.be.true;
+    });
 });

test/config/validator.js

@@ -13,6 +13,9 @@ const expect = require('chai').expect;
 const WebpackConfig = require('../../lib/WebpackConfig');
 const RuntimeConfig = require('../../lib/config/RuntimeConfig');
 const validator = require('../../lib/config/validator');
+const logger = require('../../lib/logger');
+
+logger.quiet();
 
 function createConfig() {
     const runtimeConfig = new RuntimeConfig();
@@ -65,4 +68,19 @@ describe('The validator function', () => {
             validator(config);
         }).to.throw('Don\'t enable versioning with the dev-server');
     });
+
+    it('warning with dev-server and absolute publicPath', () => {
+        const config = createConfig();
+        config.outputPath = '/tmp/public/build';
+        config.setPublicPath('https://absoluteurl.com/build');
+        config.setManifestKeyPrefix('build/');
+        config.addEntry('main', './main');
+        config.runtimeConfig.useDevServer = true;
+
+        logger.clearMessages();
+        validator(config);
+
+        expect(logger.getMessages().warning).to.have.lengthOf(1);
+        expect(logger.getMessages().warning[0]).to.include('Passing an absolute URL to setPublicPath() *and* using the dev-server can cause issues');
+    });
 });