Allow publicPath and basePath together

[gmac]

Another adjustment for the v.2 overhaul: this cleans up the roles and responsibilities of publicPath and basePath. Two specific changes:

1. The public and base path options are no longer mutually exclusive; they may be used together. This alleviates each setting's individual limitations.
2. Base path gets applies after public path so that the output files may be relativized, and then the whole build. This feels a lot more predictable than the two settings toggling each other.

This technically represents a breaking change, although there is low likelihood of consumer issues (given that there is literally no reason to include publicPath in the presence of basePath at the moment). If consumers do currently define both options and their build breaks in v2, the fix is simple: just delete publicPath.

[codecov-io]

Codecov Report

Merging #61 into master will not change coverage. The diff coverage is 100%.

@@           Coverage Diff           @@
##           master      #61   +/-   ##
=======================================
  Coverage   92.98%   92.98%           
=======================================
  Files           2        2           
  Lines          57       57           
=======================================
  Hits           53       53           
  Misses          4        4

Impacted Files	Coverage Δ
lib/plugin.js	92.85% <100%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bdb5758...7117854. Read the comment docs.

[mastilver]

👍 I think it make sense

[weaverryan]

Thanks for this @gmac!

First... I apologize for being difficult! I'm realizing that we must be doing something in the Webpack Encore library that is a bit non-traditional with our manifest :/. This PR won't yet work for us.

To use your example, suppose the manifest starts like this:

{"foo/bar.js":"foo/bar.123.js"}

If the output path is some build/ dir, then we could use basePath to transform to this:

{"build/foo/bar.js":"build/foo/bar.123.js"}

So far, we're good! But, we also allow people to set their public path to a CDN (e.g. https://coolcdn.com/build). Technically, that CDN doesn't need to be reflected in the manifest values... but it's really convenient for our users to be able to transform the manifest to this:

{"build/foo/bar.js":"https://coolcdn.com/build/foo/bar.123.js"}

And this is the part that's not possible yet. With this PR, the we would set the publicPath to the CDN and the basePath to build/, but would end up (I think) with a manifest like this:

{"build/foo/bar.js":"build/https://coolcdn.com/build/foo/bar.123.js"}

Does that make sense? I think we're coming from a community with a slightly different paradigm, so I think I'm having trouble explaining myself :). We read the manifest.json file in server-side code to convert from build/foo/bar.json to the real value that should be used in the <script> tag. Having the CDN there means the user can change their publicPath to a CDN in webpack... and their server-side code automatically starts using it without any changes (server-side templates always reference build/foo/bar.json).

Thanks!

[mastilver]

@gmac I believe @weaverryan is right

basePath should be for the key publicPath for the value

his issue was because the code is currently like that

if (publicPath) {

} else if (basePath) {

}

It would only pick up publicPath or basePath, not both

[mastilver]

I found this in the readme:

Ignored if basePath was also provided.

Which I think is wrong, no?

[weaverryan]

@mastilver are you talking about the readme on master on this PR? I wasn't sure what you meant :).

That note is accurate for the behavior on master, but was removed here. That's actually the behavior on master I'm hoping we can change :).

[mastilver]

@weaverryan I must be a bit tired :D

You are right, this PR, is fixing the fact that publicPath is being ignored ;)

[mastilver]

@gmac I think it should follow #69 instead

[mastilver]

closing is flavour of #74