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

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

Conversation

stoccc
Copy link
Contributor

@stoccc stoccc commented Feb 13, 2018

This allows empty publicPath in webpack.config.js
Provided a functional test.
See #26 (comment)

I'm new to webpack/encore/nodejs/git, so I hope I didn't make any mistake :)

@stoccc
Copy link
Contributor Author

stoccc commented Feb 13, 2018

@weaverryan I think I'll need some help with the functional test...

@stoccc
Copy link
Contributor Author

stoccc commented Feb 13, 2018

It seems that we have some problems using javascript require.ensure: the file path is not translated to the client according to the information stored in manifest.json.
Is there any config that we can edit so that the reference is generated properly?

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Hey @stoccc!

Thanks for starting this! Ok, about the require.ensure call... this is Webpack's responsibility to resolve this path correctly: it does not read from our manifest.json. file. Actually, it basically just uses the final publicPath config key to figure out the path to any code split files. So... this is a real problem: I'm not sure if relative paths can work with code-splitting. Since Webpack uses publicPath to find these files, if you use a relative path, then it will try to request a different file based on the URL of your page - e.g. http://example.com/foo versus http://example.com/foo/bar.

I found this with some quick research: https://stackoverflow.com/questions/36675286/webpack-code-splitting-relative-url

If this is correct, it confirms my believe.... because the solution they're suggesting is to override the publicPath at runtime... which is the only time we know the true path.

Can you research this a bit more? It may be that relative paths will simply not work with code splitting (unless the user does this workaround at runtime (more on this: https://webpack.js.org/guides/public-path/#on-the-fly).

// 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?

@@ -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.

@weaverryan weaverryan changed the base branch from master to main November 18, 2020 18:22
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.

None yet

2 participants