shellscape / webpack-manifest-plugin

webpack plugin for generating asset manifests
MIT License
1.44k stars 184 forks source link

Resolve paths #38

Closed Lobstrosity closed 7 years ago

Lobstrosity commented 7 years ago

When populating the manifest, resolves basePath/publicPath with key/value (to resolve parent directory references).

So /base/subfolder/../file.ext becomes /base/file.ext.

Side benefit is trailing slashes on basePath/publicPath are now optional since join takes care of that for you.

Note that it uses path.normalize(path.join()) instead of path.resolve() so that ancestors of basePath/publicPath are not exposed. (The approach could be simplified if that’s not a concern.) This results in invalid paths if an absolute path is specified with several parent directory references. E.g., resolvePath('/absolute/../..', 'file.ext') returns '/file.ext' (not valid), whereas resolvePath('relative/../..', 'file.ext') returns '../file.ext'.


My particular use case was with sass-loader plus file-loader to export CSS-referenced images.

Before this change, an image would appear in the manifest as /static/css/../images/file.ext. With this change, it appears as /static/images/file.ext.

mastilver commented 7 years ago

@Lobstrosity Could you rebase on top on master

Tests are failing on my side

mastilver commented 7 years ago

@a-x-

Tests are failing on my side

Also are you :100: it's not a breaking change?

a-x- commented 7 years ago

I not 100% sure, but I saw passed tests (after rebase as I understood) and code likes good It can breaks usage relied on buggy behavior probably.

Excuse me if you want to test it better

mastilver commented 7 years ago

fine :) can you fix the tests please

mastilver commented 7 years ago

@a-x- And next time can you squash it instead of merging the PR, it makes thinks look nicer :)

a-x- commented 7 years ago

hey there was a deep evening at here yesterday :) I see you fix tests yourself now.

I checked locally, everything passes 👍 I just don't understand why was tests in CI good

mastilver commented 7 years ago

I just don't understand why was tests in CI good

Weird, I think you got confuse with code approval checks :p

mastilver commented 7 years ago

This caused an issue see #62 , we need to find a workaround to fix this issue while keeping urls safe