-
-
Notifications
You must be signed in to change notification settings - Fork 199
Fixing Windows Support #28
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
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
6c65840
Fixing Windows support:
weaverryan 5f8ff97
Fixing a small bug when reporting the relative output path
weaverryan 162d5f5
Moving some validation logic to be more clear about the flow
weaverryan 6164d80
Tweaks thanks to Stof, including combining Unix & windows tests
weaverryan 78a0680
cs
weaverryan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| /* | ||
| * 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 path = require('path'); | ||
|
|
||
| module.exports = { | ||
| /** | ||
| * Determines the "contentBase" to use 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 | ||
| * | ||
| * 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 | ||
| // 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 ${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.`); | ||
| }, | ||
|
|
||
| /** | ||
| * 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() + path.sep, ''); | ||
| }, | ||
|
|
||
| /** | ||
| * 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 {void} | ||
| */ | ||
| 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 | ||
| * 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.replace(/\\/g, '/'); | ||
|
|
||
| // 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.`); | ||
| } | ||
| } | ||
| }; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| /* | ||
| * 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 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); | ||
| } | ||
|
|
||
| const isWindows = (process.platform === 'win32'); | ||
|
|
||
| describe('path-util getContentBase()', () => { | ||
| describe('getContentBase()', () => { | ||
| it('contentBase is calculated correctly', function() { | ||
| const config = createConfig(); | ||
| config.runtimeConfig.useDevServer = true; | ||
| config.runtimeConfig.devServerUrl = 'http://localhost:8080/'; | ||
| 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(isWindows ? 'C:\\tmp\\public' : '/tmp/public'); | ||
| }); | ||
|
|
||
| it('contentBase works ok with manifestKeyPrefix', function() { | ||
| const config = createConfig(); | ||
| config.runtimeConfig.useDevServer = true; | ||
| config.runtimeConfig.devServerUrl = 'http://localhost:8080/'; | ||
| 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(isWindows ? 'C:\\tmp\\public' : '/tmp/public'); | ||
| }); | ||
| }); | ||
|
|
||
| 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'); | ||
|
|
||
| // NOT throwing an error is the assertion | ||
| pathUtil.validatePublicPathAndManifestKeyPrefix(config); | ||
| }); | ||
|
|
||
| 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.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'); | ||
| }); | ||
|
|
||
| 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.addEntry('main', './main'); | ||
| // pretend we're installed to a subdirectory | ||
| config.setPublicPath('/subdirectory/build'); | ||
|
|
||
| expect(() => { | ||
| 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'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getRelativeOutputPath', () => { | ||
| it('basic usage', function() { | ||
| const config = createConfig(); | ||
| 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 actualPath = pathUtil.getRelativeOutputPath(config); | ||
| expect(actualPath).to.equal(isWindows ? 'public\\build' : 'public/build'); | ||
| }); | ||
| }); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if publicPath is
/subdir/build/? AFAIKpath.join(contentBase, publicPath) === outputPathwill never be true, asoutputPathwill use\everywhere, whilepath.join(contentBase, publicPath)will have some/There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct. But, if the user has a
publicPaththat is not a sub-string of theoutputPath, they will already have received an error forcing them to setmanifestKeyPrefix(you do not need to set this manually... until you deploy under a subdirectory or set publicPath to a CDN). So, this situation won't happen. I've just pushed a commit to hopefully clarify this - it's a bit complex since different parts of the system are working together. It is possible that the user will set amanifestKeyPrefixthat is not a substring of theoutputPath, and then they would see this error (because this is not allowed)