fix(karma-webpack): don't include the `os.tmpdir` (`output.publicPath`) #338

pat841 · 2018-08-27T13:14:10Z

Please check if the PR fulfills these requirements

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

This fixes issue #333 where a worker would fail to load due to a CORS issue. This change does NOT affect the write path for the webpack build, but the public path reflected in something like <base href="/_karma_webpack/">.

What is the new behavior?

The os.tmpdir() is stripped from the webpack's publicPath so that async assets are loaded correctly from a common domain avoiding CORS issues.

Does this PR introduce a breaking change?

Yes
No

If this PR contains a breaking change, please describe the following...

Impact:
Migration path for existing applications:
Github Issue(s) this is regarding:

Other information:

jsf-clabot · 2018-08-27T13:14:12Z

All committers have signed the CLA.

michael-ciniawsky · 2018-08-29T04:51:20Z

src/karma-webpack.js

@@ -69,7 +69,7 @@ function Plugin(
    // Must have the common _karma_webpack_ prefix on path here to avoid
    // https://github.com/webpack/webpack/issues/645
    webpackOptions.output.path = path.join(os.tmpdir(), '_karma_webpack_', indexPath, '/')
-    webpackOptions.output.publicPath = path.join(os.tmpdir(), '_karma_webpack_', publicPath, '/')
+    webpackOptions.output.publicPath = path.join('/_karma_webpack_', publicPath, '/')


That is simply incorrect right (in general)? Resulting in async chunks loading to fail? I'm only an admin here with limited knowledge about the current needs of a karma setup, so I need to rely on your help here :)

"That is simply incorrect right (in general)?" What do you mean by that? I believe this has been broken since PR #279 but has flown under the radar since it only breaks assets with strict CORS checking.

JayCanuck · 2018-08-30T01:41:44Z

Just wanted to add my 2cents here. Since #279 karma-webpack has been broken for myself and colleagues on Windows (Windows was resolving C:/.... into file://C:/ urls and failing for XHR). Was going to create a new issue but saw this PR. Looks like this patch fixes that issue flawlessly too. All tests resume passing with this PR branch.
Edit: looks like there's a related issue of others seeing the same Windows ajax issues that this PR solves #317

lsphillips · 2018-08-30T18:27:20Z

I was about to submit a PR, good thing I checked first :-).

Would really like to see this merged as soon as possible as this makes this project unusable for Windows users, especially as code splitting is a common practice these days.

michael-ciniawsky · 2018-09-01T02:54:18Z

cc @rynclark

ryanclark · 2018-09-01T03:30:12Z

LGTM after conflicts are resolved

…path

pat841 · 2018-09-03T16:39:26Z

@rynclark Should be all set.

JayCanuck · 2018-09-03T18:02:09Z

@rynclark Tests are failing for me with the latest commit. Looks to be because path.join is used on webpackMiddlewareOptions.publicPath, which on Windows results in backslashes (\) and that backslash-based path fails to match the forward-slash (/) requests. Can you modify that line to be a static '/_karma_webpack_/' like it was before the rebase?

pat841 · 2018-09-03T18:04:15Z

@JayCanuck Yeah I was unsure about using path.join() for that reason but wanted to stay within the re-base updates. @rynclark Can I remove the path.join() calls around publicPath setters?

JayCanuck · 2018-09-03T18:09:43Z

At least in local testing, it looks like the webpackOptions.output.publicPath = path.join(... is fine and webpack handles backslashes correctly, but for handing request public path (and all the things that it extends to), the path.join(...) on webpackMiddlewareOptions.publicPath seems to be the key problem on Windows and needs forward slashes.

pat841 · 2018-09-03T18:17:23Z

Theres also the option of .replace(/\\/g, '/');

JayCanuck · 2018-09-03T18:30:12Z

That's true, yea, though that feels like having the unnecessary path.join and replace operations for unneeded processing compared to an equivalent static string.

michael-ciniawsky · 2018-09-04T02:53:56Z

@pat841 Please leave it as is within this PR. It is a bug introduced by another recent PR and this ideally should to be adressed separately

michael-ciniawsky

@pat841 Thx

pat841 · 2018-09-04T12:52:29Z

@michael-ciniawsky Would you like me to create a new PR for the Windows fix based off of this branch?

michael-ciniawsky

Rubberstamp LGTM

@pat841 Thx

michael-ciniawsky · 2018-09-07T15:20:39Z

Released in 4.0.0-rc.2 🎉

johnnyreilly · 2018-09-24T06:33:14Z

I've just tested 4.0.0-rc.2 on Windows; still a problem I'm afraid. It's what we use in ts-loader for our execution test pack.

You can see this PR upgrades from 2.0.6 to 4.0.0-rc2 and then experiences issues:

TypeStrong/ts-loader#840

Linux is fine; Windows is not. The failing tests all relate to code splitting / dynamic imports.

I'll open a new issue to track this and link back.

jsleuth approved these changes Aug 27, 2018

View reviewed changes

michael-ciniawsky changed the title ~~fix(karma-webpack): dont include the os.tmpdir in the webpack public …~~ fix(karma-webpack): don't include the os.tmpdir in the publicPath Aug 29, 2018

michael-ciniawsky reviewed Aug 29, 2018

View reviewed changes

michael-ciniawsky added this to the 4.0.0 milestone Aug 29, 2018

michael-ciniawsky added type: Bug priority: 4 (important) pr_action: Review semver: Patch status: Pending severity: 3 (broken) labels Aug 29, 2018

michael-ciniawsky requested a review from alexander-akait August 29, 2018 04:53

michael-ciniawsky mentioned this pull request Aug 29, 2018

fix(karma-webpack): disable optimization by default (webpack >= v4.0.0) #325

Merged

3 tasks

michael-ciniawsky added priority: 2 (urgent) and removed priority: 4 (important) labels Aug 31, 2018

michael-ciniawsky changed the title ~~fix(karma-webpack): don't include the os.tmpdir in the publicPath~~ fix(karma-webpack): don't include os.tmpdir in the publicPath Aug 31, 2018

michael-ciniawsky force-pushed the next branch from dd20f5e to 5f6c7af Compare September 1, 2018 03:43

michael-ciniawsky added pr_action: Rebase and removed pr_action: Review labels Sep 1, 2018

michael-ciniawsky changed the base branch from next to master September 1, 2018 04:03

michael-ciniawsky changed the title ~~fix(karma-webpack): don't include os.tmpdir in the publicPath~~ fix(karma-webpack): don't include os.tmpdir (options.publicPath) Sep 1, 2018

pat841 force-pushed the feature/webpack-path-fix branch from 65fec5d to 49bdae1 Compare September 3, 2018 16:24

fix(karma-webpack): dont include the os.tmpdir in the webpack public …

bac4676

…path

pat841 force-pushed the feature/webpack-path-fix branch from 49bdae1 to bac4676 Compare September 3, 2018 16:31

michael-ciniawsky approved these changes Sep 4, 2018

View reviewed changes

michael-ciniawsky added pr_action: Review and removed pr_action: Rebase labels Sep 4, 2018

michael-ciniawsky requested a review from ryanclark September 4, 2018 02:55

michael-ciniawsky mentioned this pull request Sep 7, 2018

fix(karma-webpack): normalize paths (windows) #351

Merged

2 tasks

michael-ciniawsky added status: Approved and removed pr_action: Review priority: 2 (urgent) severity: 3 (broken) status: Pending labels Sep 7, 2018

michael-ciniawsky approved these changes Sep 7, 2018

View reviewed changes

michael-ciniawsky merged commit 66f4cd7 into codymikol:master Sep 7, 2018

michael-ciniawsky changed the title ~~fix(karma-webpack): don't include os.tmpdir (options.publicPath)~~ fix(karma-webpack): don't include the os.tmpdir (output.publicPath) Sep 7, 2018

michael-ciniawsky mentioned this pull request Sep 7, 2018

Dynamic import does not work on windows after 2.0.7 (re-open) #317

Closed

pat841 deleted the feature/webpack-path-fix branch September 10, 2018 12:52

johnnyreilly mentioned this pull request Sep 24, 2018

Dynamic import does not work on windows after 2.0.7 (re-open part deux) #362

Closed

andrewiggins mentioned this pull request Nov 20, 2018

Update dependencies developit/karmatic#25

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(karma-webpack): don't include the `os.tmpdir` (`output.publicPath`) #338

fix(karma-webpack): don't include the `os.tmpdir` (`output.publicPath`) #338

pat841 commented Aug 27, 2018

jsf-clabot commented Aug 27, 2018 •

edited

Loading

michael-ciniawsky Aug 29, 2018 •

edited

Loading

pat841 Aug 29, 2018

JayCanuck commented Aug 30, 2018 •

edited

Loading

lsphillips commented Aug 30, 2018

michael-ciniawsky commented Sep 1, 2018

ryanclark commented Sep 1, 2018

pat841 commented Sep 3, 2018

JayCanuck commented Sep 3, 2018 •

edited

Loading

pat841 commented Sep 3, 2018

JayCanuck commented Sep 3, 2018

pat841 commented Sep 3, 2018

JayCanuck commented Sep 3, 2018

michael-ciniawsky commented Sep 4, 2018

michael-ciniawsky left a comment

pat841 commented Sep 4, 2018 •

edited

Loading

michael-ciniawsky left a comment

michael-ciniawsky commented Sep 7, 2018

johnnyreilly commented Sep 24, 2018 •

edited

Loading

fix(karma-webpack): don't include the os.tmpdir (output.publicPath) #338

fix(karma-webpack): don't include the os.tmpdir (output.publicPath) #338

Conversation

pat841 commented Aug 27, 2018

jsf-clabot commented Aug 27, 2018 • edited Loading

michael-ciniawsky Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

pat841 Aug 29, 2018

Choose a reason for hiding this comment

JayCanuck commented Aug 30, 2018 • edited Loading

lsphillips commented Aug 30, 2018

michael-ciniawsky commented Sep 1, 2018

ryanclark commented Sep 1, 2018

pat841 commented Sep 3, 2018

JayCanuck commented Sep 3, 2018 • edited Loading

pat841 commented Sep 3, 2018

JayCanuck commented Sep 3, 2018

pat841 commented Sep 3, 2018

JayCanuck commented Sep 3, 2018

michael-ciniawsky commented Sep 4, 2018

michael-ciniawsky left a comment

Choose a reason for hiding this comment

pat841 commented Sep 4, 2018 • edited Loading

michael-ciniawsky left a comment

Choose a reason for hiding this comment

michael-ciniawsky commented Sep 7, 2018

johnnyreilly commented Sep 24, 2018 • edited Loading

fix(karma-webpack): don't include the `os.tmpdir` (`output.publicPath`) #338

fix(karma-webpack): don't include the `os.tmpdir` (`output.publicPath`) #338

jsf-clabot commented Aug 27, 2018 •

edited

Loading

michael-ciniawsky Aug 29, 2018 •

edited

Loading

JayCanuck commented Aug 30, 2018 •

edited

Loading

JayCanuck commented Sep 3, 2018 •

edited

Loading

pat841 commented Sep 4, 2018 •

edited

Loading

johnnyreilly commented Sep 24, 2018 •

edited

Loading