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

Fix publicPath to be windows compatible #366

Closed
wants to merge 3 commits into from

Conversation

gfdickinson
Copy link

This PR contains a:

  • [X ] bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

When requiring html templates that were loaded with html-loader and then file-loader, the url that was generated was not compatible with windows machines. The path.join was changing the '/' into a ''.

Breaking Changes

N/A

Additional Info

@jsf-clabot
Copy link

jsf-clabot commented Oct 16, 2018

CLA assistant check
All committers have signed the CLA.

@chriscasola
Copy link

chriscasola commented Oct 26, 2018

@michael-ciniawsky could we get this merged?

This actually doesn't fix the issue for me on windows, but I think something like this might:

webpackOptions.output.publicPath = path.join('absolute', os.tmpdir(), '_karma_webpack_', publicPath, '/');

@gfdickinson
Copy link
Author

@chriscasola how does your version with 'absolute' joined on the path fix things?

Copy link
Collaborator

@matthieu-foucault matthieu-foucault left a comment

Choose a reason for hiding this comment

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

Please undo the changes in files other than src/karma-webpack.js.

The actual change looks good to me.

@ryanclark
Copy link
Collaborator

@chriscasola can you confirm if this change/your change works on Windows (I'm unable to test), if so I agree with @matthieu-foucault

@matthieu-foucault
Copy link
Collaborator

I don't understand @chriscasola 's version. publicPath is a URL, not a filesystem path, and thus shouldn't be built using path.join

@matthieu-foucault
Copy link
Collaborator

Closing in favor of #373

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants