From 6c6584060ce8d3739287fcb01d179d2c147fb650 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Fri, 16 Jun 2017 11:43:52 -0400 Subject: [PATCH 1/5] Fixing Windows support: * Refactoring delicate path logic into path-util * Fixing logic that relied in unix-style paths * Fixed a bug inside webpack-manifest-plugin that used \ for manifest.json key paths on Windows --- lib/config-generator.js | 25 +---- lib/config/path-util.js | 110 +++++++++++++++++++++ lib/config/validator.js | 38 -------- lib/webpack/webpack-manifest-plugin.js | 7 ++ test/config-generator.js | 33 ------- test/config/path-util.js | 127 +++++++++++++++++++++++++ test/config/validator.js | 24 ----- test/functional.js | 26 ++++- 8 files changed, 271 insertions(+), 119 deletions(-) create mode 100644 lib/config/path-util.js create mode 100644 test/config/path-util.js diff --git a/lib/config-generator.js b/lib/config-generator.js index 8467c796..98452d0f 100644 --- a/lib/config-generator.js +++ b/lib/config-generator.js @@ -22,6 +22,7 @@ const missingLoaderTransformer = require('./friendly-errors/transformers/missing const missingLoaderFormatter = require('./friendly-errors/formatters/missing-loader'); const missingPostCssConfigTransformer = require('./friendly-errors/transformers/missing-postcss-config'); const missingPostCssConfigFormatter = require('./friendly-errors/formatters/missing-postcss-config'); +const pathUtil = require('./config/path-util'); class ConfigGenerator { /** @@ -275,8 +276,7 @@ class ConfigGenerator { */ let manifestPrefix = this.webpackConfig.manifestKeyPrefix; if (null === manifestPrefix) { - // by convention, we remove the opening slash on the manifest keys - manifestPrefix = this.webpackConfig.publicPath.replace(/^\//,''); + manifestPrefix = pathUtil.generateManifestKeyPrefix(this.webpackConfig); } plugins.push(new ManifestPlugin({ basePath: manifestPrefix, @@ -407,7 +407,7 @@ class ConfigGenerator { plugins.push(friendlyErrorsPlugin); if (!this.webpackConfig.useDevServer()) { - const outputPath = this.webpackConfig.outputPath.replace(this.webpackConfig.getContext() + '/', ''); + const outputPath = pathUtil.getRelativeOutputPath(this.webpackConfig); plugins.push(new AssetOutputDisplayPlugin(outputPath, friendlyErrorsPlugin)); } @@ -440,24 +440,7 @@ class ConfigGenerator { } buildDevServerConfig() { - // strip trailing slash - const outputPath = this.webpackConfig.outputPath.replace(/\/$/,''); - // use the manifestKeyPrefix if available - const publicPath = this.webpackConfig.manifestKeyPrefix ? this.webpackConfig.manifestKeyPrefix.replace(/\/$/,'') : this.webpackConfig.publicPath.replace(/\/$/,''); - - /* - * We use the intersection of the publicPath and outputPath to determine - * "document root" of the web server. For example: - * * outputPath = /var/www/public/build - * * publicPath = /build/ - * => contentBase should be /var/www/public - */ - if (outputPath.indexOf(publicPath) === -1) { - throw new Error(`Unable to determine contentBase option for webpack's devServer configuration. The publicPath (${this.webpackConfig.publicPath}) string does not exist in the outputPath (${this.webpackConfig.outputPath}), and so the "document root" cannot be determined.`); - } - - // a non-regex replace - const contentBase = outputPath.split(publicPath).join(''); + const contentBase = pathUtil.getContentBase(this.webpackConfig); return { contentBase: contentBase, diff --git a/lib/config/path-util.js b/lib/config/path-util.js new file mode 100644 index 00000000..1d3c4061 --- /dev/null +++ b/lib/config/path-util.js @@ -0,0 +1,110 @@ +/* + * This file is part of the Symfony package. + * + * (c) Fabien Potencier + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +'use strict'; + +const path = require('path'); + +module.exports = { + /** + * Determines the "contentBase" used for the devServer. + * + * @param {WebpackConfig} webpackConfig + * @return {String} + */ + getContentBase(webpackConfig) { + // strip trailing slash (for Unix or Windows) + const outputPath = webpackConfig.outputPath.replace(/\/$/,'').replace(/\\$/, ''); + // use the manifestKeyPrefix if available + const publicPath = webpackConfig.manifestKeyPrefix ? webpackConfig.manifestKeyPrefix.replace(/\/$/,'') : webpackConfig.publicPath.replace(/\/$/,''); + + /* + * We use the intersection of the publicPath (or manifestKeyPrefix) and outputPath + * to determine the "document root" of the web server. For example: + * * outputPath = /var/www/public/build + * * publicPath = /build/ + * => contentBase should be /var/www/public + */ + + // start with outputPath, then join publicPath with it, see if it equals outputPath + // in loop, do dirname on outputPath and repeat + // eventually, you (may) get to the right path + let contentBase = outputPath; + while (path.dirname(contentBase) !== contentBase) { + contentBase = path.dirname(contentBase); + + if (path.join(contentBase, publicPath) === outputPath) { + return contentBase; + } + } + + throw new Error(`Unable to determine contentBase option for webpack's devServer configuration. The publicPath (${webpackConfig.publicPath}) string does not exist in the outputPath (${webpackConfig.outputPath}), and so the "document root" cannot be determined.`); + }, + + /** + * Returns the output path, but as a relative string (e.g. web/build) + * + * @param {WebpackConfig} webpackConfig + * @return {String} + */ + getRelativeOutputPath(webpackConfig) { + return webpackConfig.outputPath.replace(webpackConfig.getContext() + '/', ''); + }, + + /** + * If the manifestKeyPrefix is not set, this uses the publicPath to generate it. + * + * Most importantly, this runs some sanity checks to make sure that it's + * ok to use the publicPath as the manifestKeyPrefix. + * + * @param {WebpackConfig} webpackConfig + * @return {String} + */ + generateManifestKeyPrefix(webpackConfig) { + if (webpackConfig.publicPath.includes('://')) { + /* + * If publicPath is absolute, you probably don't want your manifests.json + * keys to be prefixed with the CDN URL. Instead, we force you to + * choose your manifestKeyPrefix. + */ + + throw new Error('Cannot determine how to prefix the keys in manifest.json. Call Encore.setManifestKeyPrefix() to choose what path (e.g. build/) to use when building your manifest keys. This is happening because you passed an absolute URL to setPublicPath().'); + } + + let outputPath = webpackConfig.outputPath; + // for comparison purposes, change \ to / on Windows + outputPath = outputPath.split('\\').join('/'); + + // remove trailing slash on each + outputPath = outputPath.replace(/\/$/, ''); + const publicPath = webpackConfig.publicPath.replace(/\/$/, ''); + + /* + * This is a sanity check. If, for example, you are deploying + * to a subdirectory, then you might have something like this: + * outputPath = /var/www/public/build + * publicPath = /subdir/build/ + * + * In that case, you probably don't want the keys in the manifest.json + * file to be prefixed with /subdir/build - it makes more sense + * to prefix them with /build, which is the true prefix relative + * to your application (the subdirectory is a deployment detail). + * + * For that reason, we force you to choose your manifestKeyPrefix(). + */ + if (outputPath.indexOf(publicPath) === -1) { + const suggestion = publicPath.substr(publicPath.lastIndexOf('/') + 1) + '/'; + + throw new Error(`Cannot determine how to prefix the keys in manifest.json. Call Encore.setManifestKeyPrefix() to choose what path (e.g. ${suggestion}) to use when building your manifest keys. This is caused by setOutputPath() (${outputPath}) and setPublicPath() (${publicPath}) containing paths that don't seem compatible.`); + } + + // by convention, we remove the opening slash on the manifest keys + return webpackConfig.publicPath.replace(/^\//,''); + } +}; diff --git a/lib/config/validator.js b/lib/config/validator.js index 7778db1b..af21c031 100644 --- a/lib/config/validator.js +++ b/lib/config/validator.js @@ -20,8 +20,6 @@ class Validator { validate() { this._validateBasic(); - this._validatePublicPathConfig(); - this._validateDevServer(); } @@ -39,42 +37,6 @@ class Validator { } } - _validatePublicPathConfig() { - if (this.webpackConfig.publicPath.includes('://') && !this.webpackConfig.manifestKeyPrefix) { - /* - * If publicPath is absolute, you probably don't want your manifests.json - * keys to be prefixed with the CDN URL. Instead, we force you to - * choose your manifestKeyPrefix. - */ - - throw new Error('Cannot determine how to prefix the keys in manifest.json. Call Encore.setManifestKeyPrefix() to choose what path (e.g. build/) to use when building your manifest keys. This is happening because you passed an absolute URL to setPublicPath().'); - } - - if (!this.webpackConfig.manifestKeyPrefix) { - const outputPath = this.webpackConfig.outputPath.replace(/\/$/, ''); - const publicPath = this.webpackConfig.publicPath.replace(/\/$/, ''); - - /* - * This is a sanity check. If, for example, you are deploying - * to a subdirectory, then you might have something like this: - * outputPath = /var/www/public/build - * publicPath = /subdir/build/ - * - * In that case, you probably don't want the keys in the manifest.json - * file to be prefixed with /subdir/build - it makes more sense - * to prefix them with /build, which is the true prefix relative - * to your application (the subdirectory is a deployment detail). - * - * For that reason, we force you to choose your manifestKeyPrefix(). - */ - if (outputPath.indexOf(publicPath) === -1) { - const suggestion = publicPath.substr(publicPath.lastIndexOf('/') + 1) + '/'; - - throw new Error(`Cannot determine how to prefix the keys in manifest.json. Call Encore.setManifestKeyPrefix() to choose what path (e.g. ${suggestion}) to use when building your manifest keys. This is caused by setOutputPath() and setPublicPath() containing paths that don't seem compatible.`); - } - } - } - _validateDevServer() { if (this.webpackConfig.useVersioning && this.webpackConfig.useDevServer()) { throw new Error('Don\'t enable versioning with the dev-server. A good setting is Encore.enableVersioning(Encore.isProduction()).'); diff --git a/lib/webpack/webpack-manifest-plugin.js b/lib/webpack/webpack-manifest-plugin.js index 49ee37d6..50c9fd88 100644 --- a/lib/webpack/webpack-manifest-plugin.js +++ b/lib/webpack/webpack-manifest-plugin.js @@ -45,6 +45,13 @@ ManifestPlugin.prototype.apply = function(compiler) { path.dirname(file), path.basename(module.userRequest) ); + /* *** MODIFICATION START *** */ + // for Windows, we want the keys to use /, not \ + // (and path.join will obviously use \ in Windows) + if (process.platform === 'win32') { + moduleAssets[file] = moduleAssets[file].split('\\').join('/'); + } + /* *** MODIFICATION END *** */ }); }); diff --git a/test/config-generator.js b/test/config-generator.js index b8fbbcfe..7d387f72 100644 --- a/test/config-generator.js +++ b/test/config-generator.js @@ -466,39 +466,6 @@ describe('The config-generator function', () => { const actualConfig = configGenerator(config); expect(actualConfig.devServer).to.be.undefined; }); - - it('contentBase is calculated correctly', () => { - const config = createConfig(); - config.runtimeConfig.useDevServer = true; - config.runtimeConfig.devServerUrl = 'http://localhost:8080/'; - config.outputPath = '/tmp/public/build'; - config.setPublicPath('/build/'); - config.addEntry('main', './main'); - - const actualConfig = configGenerator(config); - // contentBase should point to the "document root", which - // is calculated as outputPath, but without the publicPath portion - expect(actualConfig.devServer.contentBase).to.equal('/tmp/public'); - expect(actualConfig.devServer.publicPath).to.equal('/build/'); - - }); - - it('contentBase works ok with manifestKeyPrefix', () => { - const config = createConfig(); - config.runtimeConfig.useDevServer = true; - config.runtimeConfig.devServerUrl = 'http://localhost:8080/'; - config.outputPath = '/tmp/public/build'; - config.setPublicPath('/subdirectory/build'); - // this "fixes" the incompatibility between outputPath and publicPath - config.setManifestKeyPrefix('/build/'); - config.addEntry('main', './main'); - - const actualConfig = configGenerator(config); - // contentBase should point to the "document root", which - // is calculated as outputPath, but without the publicPath portion - expect(actualConfig.devServer.contentBase).to.equal('/tmp/public'); - expect(actualConfig.devServer.publicPath).to.equal('/subdirectory/build/'); - }); }); describe('test for addPlugin config', () => { diff --git a/test/config/path-util.js b/test/config/path-util.js new file mode 100644 index 00000000..05e0b142 --- /dev/null +++ b/test/config/path-util.js @@ -0,0 +1,127 @@ +/* + * This file is part of the Symfony package. + * + * (c) Fabien Potencier + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +'use strict'; + +const expect = require('chai').expect; +const WebpackConfig = require('../../lib/WebpackConfig'); +const RuntimeConfig = require('../../lib/config/RuntimeConfig'); +const pathUtil = require('../../lib/config/path-util'); +const process = require('process'); + +function createConfig() { + const runtimeConfig = new RuntimeConfig(); + runtimeConfig.context = __dirname; + runtimeConfig.environment = 'dev'; + runtimeConfig.babelRcFileExists = false; + + return new WebpackConfig(runtimeConfig); +} + +/** + * Some tests are very specific to different operating systems. + * We use this to only run them when needed. + * + * @returns {boolean} + */ +function isWindows() { + return process.platform === 'win32'; +} + +describe('path-util getContentBase()', () => { + describe('getContentBase()', () => { + it('contentBase is calculated correctly', function() { + if (isWindows()) { + this.skip(); + } + + const config = createConfig(); + config.runtimeConfig.useDevServer = true; + config.runtimeConfig.devServerUrl = 'http://localhost:8080/'; + config.outputPath = '/tmp/public/build'; + config.setPublicPath('/build/'); + config.addEntry('main', './main'); + + const actualContentBase = pathUtil.getContentBase(config); + // contentBase should point to the "document root", which + // is calculated as outputPath, but without the publicPath portion + expect(actualContentBase).to.equal('/tmp/public'); + }); + + it('contentBase works ok with manifestKeyPrefix', function() { + if (isWindows()) { + this.skip(); + } + + const config = createConfig(); + config.runtimeConfig.useDevServer = true; + config.runtimeConfig.devServerUrl = 'http://localhost:8080/'; + config.outputPath = '/tmp/public/build'; + config.setPublicPath('/subdirectory/build'); + // this "fixes" the incompatibility between outputPath and publicPath + config.setManifestKeyPrefix('/build/'); + config.addEntry('main', './main'); + + const actualContentBase = pathUtil.getContentBase(config); + expect(actualContentBase).to.equal('/tmp/public'); + }); + + it('contentBase is calculated correctly on Windows', function() { + if (!isWindows()) { + this.skip(); + } + + const config = createConfig(); + config.runtimeConfig.useDevServer = true; + config.runtimeConfig.devServerUrl = 'http://localhost:8080/'; + config.outputPath = 'C:\\projects\\webpack-encore\\public\\build'; + config.setPublicPath('/build/'); + config.addEntry('main', './main'); + + const actualContentBase = pathUtil.getContentBase(config); + expect(actualContentBase).to.equal('C:\\projects\\webpack-encore\\public'); + }); + }); + + describe('generateManifestKeyPrefix', () => { + it('manifestKeyPrefix is correctly not required on windows', () => { + const config = createConfig(); + config.outputPath = 'C:\\projects\\webpack-encore\\web\\build'; + config.setPublicPath('/build/'); + config.addEntry('main', './main'); + + const actualPrefix = pathUtil.generateManifestKeyPrefix(config); + expect(actualPrefix).to.equal('build/'); + }); + + it('with absolute publicPath, manifestKeyPrefix must be set', () => { + const config = createConfig(); + config.outputPath = '/tmp/public/build'; + config.setPublicPath('/build'); + config.addEntry('main', './main'); + config.setPublicPath('https://cdn.example.com'); + + expect(() => { + pathUtil.generateManifestKeyPrefix(config); + }).to.throw('Cannot determine how to prefix the keys in manifest.json. Call Encore.setManifestKeyPrefix() to choose what path (e.g. build/) to use'); + }); + + it('when outputPath and publicPath are incompatible, manifestKeyPrefix must be set', () => { + const config = createConfig(); + config.outputPath = '/tmp/public/build'; + config.addEntry('main', './main'); + // pretend we're installed to a subdirectory + config.setPublicPath('/subdirectory/build'); + + expect(() => { + pathUtil.generateManifestKeyPrefix(config); + }).to.throw('Cannot determine how to prefix the keys in manifest.json. Call Encore.setManifestKeyPrefix() to choose what path (e.g. build/) to use'); + }); + }); +}); diff --git a/test/config/validator.js b/test/config/validator.js index 023e03b2..9ab2810c 100644 --- a/test/config/validator.js +++ b/test/config/validator.js @@ -53,30 +53,6 @@ describe('The validator function', () => { }).to.throw('Missing public path'); }); - it('with absolute publicPath, manifestKeyPrefix must be set', () => { - const config = createConfig(); - config.outputPath = '/tmp/public/build'; - config.setPublicPath('/build'); - config.addEntry('main', './main'); - config.setPublicPath('https://cdn.example.com'); - - expect(() => { - validator(config); - }).to.throw('Cannot determine how to prefix the keys in manifest.json. Call Encore.setManifestKeyPrefix() to choose what path (e.g. build/) to use'); - }); - - it('when outputPath and publicPath are incompatible, manifestKeyPrefix must be set', () => { - const config = createConfig(); - config.outputPath = '/tmp/public/build'; - config.addEntry('main', './main'); - // pretend we're installed to a subdirectory - config.setPublicPath('/subdirectory/build'); - - expect(() => { - validator(config); - }).to.throw('Cannot determine how to prefix the keys in manifest.json. Call Encore.setManifestKeyPrefix() to choose what path (e.g. build/) to use'); - }); - it('cannot use versioning with webpack-dev-server', () => { const config = createConfig(); config.outputPath = '/tmp/public/build'; diff --git a/test/functional.js b/test/functional.js index 7e69fd60..433c532a 100644 --- a/test/functional.js +++ b/test/functional.js @@ -47,17 +47,25 @@ describe('Functional tests using webpack', function() { describe('Basic scenarios', () => { - it('Builds a simple .js file + manifest.json', (done) => { + it('Builds a few simple entries file + manifest.json', (done) => { const config = createWebpackConfig('web/build', 'dev'); config.addEntry('main', './js/no_require'); + config.addStyleEntry('font', './css/roboto_font.css'); + config.addStyleEntry('bg', './css/another_bg_image.css'); config.setPublicPath('/build'); testSetup.runWebpack(config, (webpackAssert) => { // should have a main.js file // should have a manifest.json with public/main.js - expect(config.outputPath).to.be.a.directory() - .with.files(['main.js', 'manifest.json']); + expect(config.outputPath).to.be.a.directory().with.deep.files([ + 'main.js', + 'font.css', + 'bg.css', + 'fonts/Roboto.woff2', + 'images/symfony_logo.png', + 'manifest.json' + ]); // check that main.js has the correct contents webpackAssert.assertOutputFileContains( @@ -73,6 +81,18 @@ describe('Functional tests using webpack', function() { 'build/main.js', '/build/main.js' ); + webpackAssert.assertManifestPath( + 'build/font.css', + '/build/font.css' + ); + webpackAssert.assertManifestPath( + 'build/fonts/Roboto.woff2', + '/build/fonts/Roboto.woff2' + ); + webpackAssert.assertManifestPath( + 'build/images/symfony_logo.png', + '/build/images/symfony_logo.png' + ); done(); }); From 5f8ff971eb366877f88ff87add35a03e3e95384e Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Tue, 20 Jun 2017 12:51:41 -0400 Subject: [PATCH 2/5] Fixing a small bug when reporting the relative output path --- lib/config/path-util.js | 2 +- test/config/path-util.js | 28 ++++++++++++++++++++++++++++ test/functional.js | 2 +- 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/lib/config/path-util.js b/lib/config/path-util.js index 1d3c4061..692a0119 100644 --- a/lib/config/path-util.js +++ b/lib/config/path-util.js @@ -54,7 +54,7 @@ module.exports = { * @return {String} */ getRelativeOutputPath(webpackConfig) { - return webpackConfig.outputPath.replace(webpackConfig.getContext() + '/', ''); + return webpackConfig.outputPath.replace(webpackConfig.getContext() + path.sep, ''); }, /** diff --git a/test/config/path-util.js b/test/config/path-util.js index 05e0b142..fbd280cd 100644 --- a/test/config/path-util.js +++ b/test/config/path-util.js @@ -124,4 +124,32 @@ describe('path-util getContentBase()', () => { }).to.throw('Cannot determine how to prefix the keys in manifest.json. Call Encore.setManifestKeyPrefix() to choose what path (e.g. build/) to use'); }); }); + + describe('getRelativeOutputPath', () => { + it('Works with Unix paths', function() { + if (isWindows()) { + this.skip(); + } + + const config = createConfig(); + config.runtimeConfig.context = '/tmp/webpack-encore'; + config.outputPath = '/tmp/webpack-encore/public/build'; + + const actualPath = pathUtil.getRelativeOutputPath(config); + expect(actualPath).to.equal('public/build'); + }); + + it('Works with Windows paths', function() { + if (!isWindows()) { + this.skip(); + } + + const config = createConfig(); + config.runtimeConfig.context = 'C:\\projects\\webpack-encore'; + config.outputPath = 'C:\\projects\\webpack-encore\\public\\build'; + + const actualPath = pathUtil.getRelativeOutputPath(config); + expect(actualPath).to.equal('public\\build'); + }); + }); }); diff --git a/test/functional.js b/test/functional.js index 433c532a..c3522f56 100644 --- a/test/functional.js +++ b/test/functional.js @@ -39,7 +39,7 @@ function convertToManifestPath(assetSrc, webpackConfig) { describe('Functional tests using webpack', function() { // being functional tests, these can take quite long - this.timeout(5000); + this.timeout(8000); after(() => { testSetup.emptyTmpDir(); From 162d5f5ec498e0bdb60ec09f3f0ea75187476e57 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Wed, 21 Jun 2017 10:04:49 -0400 Subject: [PATCH 3/5] Moving some validation logic to be more clear about the flow --- lib/config-generator.js | 3 ++- lib/config/path-util.js | 19 ++++++++++++------- lib/config/validator.js | 8 ++++++++ test/config/path-util.js | 10 +++++----- 4 files changed, 27 insertions(+), 13 deletions(-) diff --git a/lib/config-generator.js b/lib/config-generator.js index 98452d0f..450d1e04 100644 --- a/lib/config-generator.js +++ b/lib/config-generator.js @@ -276,7 +276,8 @@ class ConfigGenerator { */ let manifestPrefix = this.webpackConfig.manifestKeyPrefix; if (null === manifestPrefix) { - manifestPrefix = pathUtil.generateManifestKeyPrefix(this.webpackConfig); + // by convention, we remove the opening slash on the manifest keys + manifestPrefix = this.webpackConfig.publicPath.replace(/^\//,''); } plugins.push(new ManifestPlugin({ basePath: manifestPrefix, diff --git a/lib/config/path-util.js b/lib/config/path-util.js index 692a0119..92f8eccc 100644 --- a/lib/config/path-util.js +++ b/lib/config/path-util.js @@ -13,7 +13,7 @@ const path = require('path'); module.exports = { /** - * Determines the "contentBase" used for the devServer. + * Determines the "contentBase" to use for the devServer. * * @param {WebpackConfig} webpackConfig * @return {String} @@ -30,6 +30,10 @@ module.exports = { * * outputPath = /var/www/public/build * * publicPath = /build/ * => contentBase should be /var/www/public + * + * At this point, if the publicPath is non-standard (e.g. it contains + * a sub-directory or is absolute), then the user will already see + * an error that they must set the manifestKeyPrefix. */ // start with outputPath, then join publicPath with it, see if it equals outputPath @@ -44,7 +48,7 @@ module.exports = { } } - throw new Error(`Unable to determine contentBase option for webpack's devServer configuration. The publicPath (${webpackConfig.publicPath}) string does not exist in the outputPath (${webpackConfig.outputPath}), and so the "document root" cannot be determined.`); + throw new Error(`Unable to determine contentBase option for webpack's devServer configuration. The ${webpackConfig.manifestKeyPrefix ? 'manifestKeyPrefix' : 'publicPath'} (${webpackConfig.manifestKeyPrefix ? webpackConfig.manifestKeyPrefix : webpackConfig.publicPath}) string does not exist in the outputPath (${webpackConfig.outputPath}), and so the "document root" cannot be determined.`); }, /** @@ -64,9 +68,13 @@ module.exports = { * ok to use the publicPath as the manifestKeyPrefix. * * @param {WebpackConfig} webpackConfig - * @return {String} */ - generateManifestKeyPrefix(webpackConfig) { + validatePublicPathAndManifestKeyPrefix(webpackConfig) { + if (webpackConfig.manifestKeyPrefix !== null) { + // nothing to check - they have manually set the key prefix + return; + } + if (webpackConfig.publicPath.includes('://')) { /* * If publicPath is absolute, you probably don't want your manifests.json @@ -103,8 +111,5 @@ module.exports = { throw new Error(`Cannot determine how to prefix the keys in manifest.json. Call Encore.setManifestKeyPrefix() to choose what path (e.g. ${suggestion}) to use when building your manifest keys. This is caused by setOutputPath() (${outputPath}) and setPublicPath() (${publicPath}) containing paths that don't seem compatible.`); } - - // by convention, we remove the opening slash on the manifest keys - return webpackConfig.publicPath.replace(/^\//,''); } }; diff --git a/lib/config/validator.js b/lib/config/validator.js index af21c031..14f6952c 100644 --- a/lib/config/validator.js +++ b/lib/config/validator.js @@ -9,6 +9,8 @@ 'use strict'; +const pathUtil = require('./path-util'); + class Validator { /** * @param {WebpackConfig} webpackConfig @@ -20,6 +22,8 @@ class Validator { validate() { this._validateBasic(); + this._validatePublicPathAndManifestKeyPrefix(); + this._validateDevServer(); } @@ -37,6 +41,10 @@ class Validator { } } + _validatePublicPathAndManifestKeyPrefix() { + pathUtil.validatePublicPathAndManifestKeyPrefix(this.webpackConfig); + } + _validateDevServer() { if (this.webpackConfig.useVersioning && this.webpackConfig.useDevServer()) { throw new Error('Don\'t enable versioning with the dev-server. A good setting is Encore.enableVersioning(Encore.isProduction()).'); diff --git a/test/config/path-util.js b/test/config/path-util.js index fbd280cd..0a0c0793 100644 --- a/test/config/path-util.js +++ b/test/config/path-util.js @@ -89,15 +89,15 @@ describe('path-util getContentBase()', () => { }); }); - describe('generateManifestKeyPrefix', () => { + describe('validatePublicPathAndManifestKeyPrefix', () => { it('manifestKeyPrefix is correctly not required on windows', () => { const config = createConfig(); config.outputPath = 'C:\\projects\\webpack-encore\\web\\build'; config.setPublicPath('/build/'); config.addEntry('main', './main'); - const actualPrefix = pathUtil.generateManifestKeyPrefix(config); - expect(actualPrefix).to.equal('build/'); + // NOT throwing an error is the assertion + pathUtil.validatePublicPathAndManifestKeyPrefix(config); }); it('with absolute publicPath, manifestKeyPrefix must be set', () => { @@ -108,7 +108,7 @@ describe('path-util getContentBase()', () => { config.setPublicPath('https://cdn.example.com'); expect(() => { - pathUtil.generateManifestKeyPrefix(config); + pathUtil.validatePublicPathAndManifestKeyPrefix(config); }).to.throw('Cannot determine how to prefix the keys in manifest.json. Call Encore.setManifestKeyPrefix() to choose what path (e.g. build/) to use'); }); @@ -120,7 +120,7 @@ describe('path-util getContentBase()', () => { config.setPublicPath('/subdirectory/build'); expect(() => { - pathUtil.generateManifestKeyPrefix(config); + pathUtil.validatePublicPathAndManifestKeyPrefix(config); }).to.throw('Cannot determine how to prefix the keys in manifest.json. Call Encore.setManifestKeyPrefix() to choose what path (e.g. build/) to use'); }); }); From 6164d80fa2b447d32b659124536e469e1b24a82f Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Wed, 21 Jun 2017 10:12:54 -0400 Subject: [PATCH 4/5] Tweaks thanks to Stof, including combining Unix & windows tests --- lib/config/path-util.js | 2 +- lib/webpack/webpack-manifest-plugin.js | 2 +- test/config/path-util.js | 73 ++++++-------------------- 3 files changed, 17 insertions(+), 60 deletions(-) diff --git a/lib/config/path-util.js b/lib/config/path-util.js index 92f8eccc..f0d0f5e1 100644 --- a/lib/config/path-util.js +++ b/lib/config/path-util.js @@ -87,7 +87,7 @@ module.exports = { let outputPath = webpackConfig.outputPath; // for comparison purposes, change \ to / on Windows - outputPath = outputPath.split('\\').join('/'); + outputPath = outputPath.replace(/\\/g, '/'); // remove trailing slash on each outputPath = outputPath.replace(/\/$/, ''); diff --git a/lib/webpack/webpack-manifest-plugin.js b/lib/webpack/webpack-manifest-plugin.js index 50c9fd88..796e774f 100644 --- a/lib/webpack/webpack-manifest-plugin.js +++ b/lib/webpack/webpack-manifest-plugin.js @@ -49,7 +49,7 @@ ManifestPlugin.prototype.apply = function(compiler) { // for Windows, we want the keys to use /, not \ // (and path.join will obviously use \ in Windows) if (process.platform === 'win32') { - moduleAssets[file] = moduleAssets[file].split('\\').join('/'); + moduleAssets[file] = moduleAssets[file].replace(/\\/g, '/'); } /* *** MODIFICATION END *** */ }); diff --git a/test/config/path-util.js b/test/config/path-util.js index 0a0c0793..fbded574 100644 --- a/test/config/path-util.js +++ b/test/config/path-util.js @@ -24,68 +24,36 @@ function createConfig() { return new WebpackConfig(runtimeConfig); } -/** - * Some tests are very specific to different operating systems. - * We use this to only run them when needed. - * - * @returns {boolean} - */ -function isWindows() { - return process.platform === 'win32'; -} +const isWindows = (process.platform === 'win32'); describe('path-util getContentBase()', () => { describe('getContentBase()', () => { it('contentBase is calculated correctly', function() { - if (isWindows()) { - this.skip(); - } - const config = createConfig(); config.runtimeConfig.useDevServer = true; config.runtimeConfig.devServerUrl = 'http://localhost:8080/'; - config.outputPath = '/tmp/public/build'; + config.outputPath = isWindows ? 'C:\\tmp\\public\\build': '/tmp/public/build'; config.setPublicPath('/build/'); config.addEntry('main', './main'); const actualContentBase = pathUtil.getContentBase(config); // contentBase should point to the "document root", which // is calculated as outputPath, but without the publicPath portion - expect(actualContentBase).to.equal('/tmp/public'); + expect(actualContentBase).to.equal(isWindows ? 'C:\\tmp\\public' : '/tmp/public'); }); it('contentBase works ok with manifestKeyPrefix', function() { - if (isWindows()) { - this.skip(); - } - const config = createConfig(); config.runtimeConfig.useDevServer = true; config.runtimeConfig.devServerUrl = 'http://localhost:8080/'; - config.outputPath = '/tmp/public/build'; + config.outputPath = isWindows ? 'C:\\tmp\\public\\build' : '/tmp/public/build'; config.setPublicPath('/subdirectory/build'); // this "fixes" the incompatibility between outputPath and publicPath config.setManifestKeyPrefix('/build/'); config.addEntry('main', './main'); const actualContentBase = pathUtil.getContentBase(config); - expect(actualContentBase).to.equal('/tmp/public'); - }); - - it('contentBase is calculated correctly on Windows', function() { - if (!isWindows()) { - this.skip(); - } - - const config = createConfig(); - config.runtimeConfig.useDevServer = true; - config.runtimeConfig.devServerUrl = 'http://localhost:8080/'; - config.outputPath = 'C:\\projects\\webpack-encore\\public\\build'; - config.setPublicPath('/build/'); - config.addEntry('main', './main'); - - const actualContentBase = pathUtil.getContentBase(config); - expect(actualContentBase).to.equal('C:\\projects\\webpack-encore\\public'); + expect(actualContentBase).to.equal(isWindows ? 'C:\\tmp\\public' : '/tmp/public'); }); }); @@ -114,7 +82,8 @@ describe('path-util getContentBase()', () => { it('when outputPath and publicPath are incompatible, manifestKeyPrefix must be set', () => { const config = createConfig(); - config.outputPath = '/tmp/public/build'; + + config.outputPath = isWindows ? 'C:\\tmp\\public\\build' : '/tmp/public/build'; config.addEntry('main', './main'); // pretend we're installed to a subdirectory config.setPublicPath('/subdirectory/build'); @@ -126,30 +95,18 @@ describe('path-util getContentBase()', () => { }); describe('getRelativeOutputPath', () => { - it('Works with Unix paths', function() { - if (isWindows()) { - this.skip(); - } - + it('basic usage', function() { const config = createConfig(); - config.runtimeConfig.context = '/tmp/webpack-encore'; - config.outputPath = '/tmp/webpack-encore/public/build'; - - const actualPath = pathUtil.getRelativeOutputPath(config); - expect(actualPath).to.equal('public/build'); - }); - - it('Works with Windows paths', function() { - if (!isWindows()) { - this.skip(); + if (isWindows) { + config.runtimeConfig.context = 'C:\\projects\\webpack-encore'; + config.outputPath = 'C:\\projects\\webpack-encore\\public\\build'; + } else { + config.runtimeConfig.context = '/tmp/webpack-encore'; + config.outputPath = '/tmp/webpack-encore/public/build'; } - const config = createConfig(); - config.runtimeConfig.context = 'C:\\projects\\webpack-encore'; - config.outputPath = 'C:\\projects\\webpack-encore\\public\\build'; - const actualPath = pathUtil.getRelativeOutputPath(config); - expect(actualPath).to.equal('public\\build'); + expect(actualPath).to.equal(isWindows ? 'public\\build' : 'public/build'); }); }); }); From 78a06803145d20d110d768ba30d8e7d920752981 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Wed, 21 Jun 2017 11:01:21 -0400 Subject: [PATCH 5/5] cs --- lib/config/path-util.js | 1 + test/config/path-util.js | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/config/path-util.js b/lib/config/path-util.js index f0d0f5e1..21380184 100644 --- a/lib/config/path-util.js +++ b/lib/config/path-util.js @@ -68,6 +68,7 @@ module.exports = { * ok to use the publicPath as the manifestKeyPrefix. * * @param {WebpackConfig} webpackConfig + * @return {void} */ validatePublicPathAndManifestKeyPrefix(webpackConfig) { if (webpackConfig.manifestKeyPrefix !== null) { diff --git a/test/config/path-util.js b/test/config/path-util.js index fbded574..779a8c3f 100644 --- a/test/config/path-util.js +++ b/test/config/path-util.js @@ -32,7 +32,7 @@ describe('path-util getContentBase()', () => { const config = createConfig(); config.runtimeConfig.useDevServer = true; config.runtimeConfig.devServerUrl = 'http://localhost:8080/'; - config.outputPath = isWindows ? 'C:\\tmp\\public\\build': '/tmp/public/build'; + config.outputPath = isWindows ? 'C:\\tmp\\public\\build' : '/tmp/public/build'; config.setPublicPath('/build/'); config.addEntry('main', './main'); @@ -83,7 +83,7 @@ describe('path-util getContentBase()', () => { it('when outputPath and publicPath are incompatible, manifestKeyPrefix must be set', () => { const config = createConfig(); - config.outputPath = isWindows ? 'C:\\tmp\\public\\build' : '/tmp/public/build'; + config.outputPath = isWindows ? 'C:\\tmp\\public\\build' : '/tmp/public/build'; config.addEntry('main', './main'); // pretend we're installed to a subdirectory config.setPublicPath('/subdirectory/build');