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.

Sign up for GitHub

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

Jump to bottom

These changes allow empty public path in webpack.config.js #264

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

These changes allow empty public path in webpack.config.js #264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca694ac
These changes allow empty public path in webpack.config.js
Feb 13, 2018
3bd6335
Fixed document root in functional test
Feb 13, 2018
9a0b382
Fix paths in functional tests
Feb 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions lib/WebpackConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,11 @@ class WebpackConfig {
logger.warning('The value passed to setPublicPath() should *usually* start with "/" or be a full URL (http://...). If you\'re not sure, then you should probably change your public path and make this message disappear.');
}

// guarantee a single trailing slash
publicPath = publicPath.replace(/\/$/,'');
publicPath = publicPath + '/';
if (publicPath !== '') {
// guarantee a single trailing slash
publicPath = publicPath.replace(/\/$/, '');
publicPath = publicPath + '/';
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to move this inside of the first if statement? I mean: if the path is not absolute (https://....) or starting with a slash (/build), then we should do this? Or is there something special about an empty publicPath?


this.publicPath = publicPath;
}
Expand Down
29 changes: 29 additions & 0 deletions test/functional.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,35 @@ describe('Functional tests using webpack', function() {
});
});

it('Deploying to an unknown (at compile-time) subdirectory is no problem', (done) => {
const config = createWebpackConfig('public/build', 'dev');
config.addEntry('main', './js/code_splitting');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also include an entry that loads a CSS file normally (i.e. without code splitting) that includes a path to an image or a font. Then, after running webpack, we can manually check the contents of the final CSS file to make sure that the path to that image is correct. As I understand it, THAT is the biggest problem we're having with relative paths: the paths to images/fonts in CSS files was being generated incorrectly.

Unfortunately, the assertResourcesLoadedCorrectly method you're using below works for JS only - that's why we need to test the contents of the file.

config.setPublicPath('');
config.setManifestKeyPrefix('build/');

testSetup.runWebpack(config, (webpackAssert) => {
webpackAssert.assertManifestPath(
'build/main.js',
'build/main.js'
);

testSetup.requestTestPage(
path.join(config.getContext(), 'public'),
[
convertToManifestPath('build/main.js', config)
],
(browser) => {
webpackAssert.assertResourcesLoadedCorrectly(browser, [
'http://127.0.0.1:8080/build/0.js',
'http://127.0.0.1:8080/build/main.js'
]);

done();
}
);
});
});

it('Empty manifestKeyPrefix is allowed', (done) => {
const config = createWebpackConfig('build', 'dev');
config.addEntry('main', './js/code_splitting');
Expand Down