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

Proper url even on windows #26

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

Conversation

visma-sorinbroscaru
Copy link

I noticed that on windows the url are actually rendered with \ instead of proper url /.
Test pass, as far as I can tell. ( On windows I had to adapt the tests expected strings to something account for platform separator.

expect(copyFileSyncMock).nthCalledWith(2, "test2.png", "dist/test2.png".replace(/\//g, sep)); //line 298 in smartAsset.test.js

in order to make it work. Since paths on windows are \\ instead of the / that is used in unix paths.

This fix should fix following scenarios:

//rollup.config.js
// ...
smartAsset({
    useHash: false,
    keepName: true,
    rebasePath: resolve(process.cwd(), 'node_modules'),
    publicPath: `/my/public/path/vendors`
}),
//...

FROM:

// compiled before changes
/* loaded by smart-asset */
var dottedBlue = "/my/public/path/vendors/@vismaux\\nordic-cool\\dist\\img\\spinners\\spinner_dotted_blue_light.svg";

TO:

// compiled after changes
/* loaded by smart-asset */
var dottedBlue = "/my/public/path/vendors/@vismaux/nordic-cool/dist/img/spinners/spinner_dotted_blue_light.svg";

Note: it was not really affecting the loading of the resource, however, I prefer universal url format style in this case.
Also note: As I looked into the copy mode, I realise that normalizeSlashes does nothing for it, however, if there will ever be the case where, just like @rollup/plugin-url, a [dirname] naming option will be included, the url should still look nice ( i hope so )

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.

1 participant