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

Replace error throwing with logger warning when publicPath is not absolute #196

Merged
merged 4 commits into from
Jan 30, 2018
Merged

Conversation

andyexeter
Copy link
Contributor

@andyexeter andyexeter commented Nov 8, 2017

There are valid use cases for setting the publicPath to something not starting with a forward slash as discussed in #88 and #26

Throwing an error and stopping compilation is overkill for this and beyond the remit of webpack encore, so I propose this PR where a warning is given to the user instead and the setPublicPath method falls in line with behaviour of setManifestKeyPrefix

Copy link
Collaborator

@Lyrkan Lyrkan left a comment

Choose a reason for hiding this comment

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

Hi @andyexeter,

I agree with you on that one. Throwing an error there is a tad violent, even if using a relative path seems to cause other issues (see this comment in #26).

@weaverryan
Copy link
Member

Since a lot of people are asking about the relative path stuff, I also think we should work on support for it (we just need some time to play with it & experiment - Webpack itself may have some issues with relative paths).

The question is - should we merge this now when we know there is a legit issue (#26 (comment)) that needs to be figured out?

@Lyrkan
Copy link
Collaborator

Lyrkan commented Nov 9, 2017

In my opinion merging it now won't be a big issue since the user would still be warned, and as @andyexeter said we already kinda do the same thing in setManifestKeyPrefix:

setManifestKeyPrefix(manifestKeyPrefix) {
/*
* Normally, we make sure that the manifest keys don't start
* with an opening "/" ever... for consistency. If you need
* to manually specify the manifest key (e.g. because you're
* publicPath is absolute), it's easy to accidentally add
* an opening slash (thereby changing your key prefix) without
* intending to. Hence, the warning.
*/
if (manifestKeyPrefix.indexOf('/') === 0) {
logger.warning(`The value passed to setManifestKeyPrefix "${manifestKeyPrefix}" starts with "/". This is allowed, but since the key prefix does not normally start with a "/", you may have just changed the prefix accidentally.`);
}

@andyexeter
Copy link
Contributor Author

andyexeter commented Nov 28, 2017

Hey @weaverryan - do you think you could take a look at merging this?

The issue you referenced in #26 wouldn't change whether this was merged or not, and the warning would still let people know they could experience issues when using a relative path.

It should be down to the end user to decide whether they want to use a relative path rather than being forced not to by the plugin.

@kevinpapst
Copy link

kevinpapst commented Jan 28, 2018

Would be great if you look into this issue again @andyexeter and @weaverryan.
I am working on migrating an open source app to Symfony 4 and webpack-encore, but I am not able to release an app build that runs inside a subdirectory and on domain-level.
I simply don't know where my user will deploy the app and no matter how I generate the files, 50% of my user will complain about missing assets.

@weaverryan weaverryan merged commit 8727b42 into symfony:master Jan 30, 2018
@weaverryan
Copy link
Member

Merged and I've just tagged 0.17.2. If you use this feature, I would appreciate it if you posted any feedback or "findings" on #26 so that we can determine of the relative paths work, or if it causes issues.

Thanks!

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.

4 participants