-
Notifications
You must be signed in to change notification settings - Fork 222
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(karma-webpack): normalize paths (windows
)
#351
Conversation
@@ -383,9 +387,6 @@ function createPreprocesor(/* config.basePath */ basePath, webpackPlugin) { | |||
throw err; | |||
} | |||
|
|||
const outputPath = webpackPlugin.outputs.get(filename); | |||
file.path = path.join(basePath, outputPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How comes you removed this? It's needed to rewrite the asset path for karma to point to .js
if the file extension was .ts
for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, but with this, just for simple js file, the karma web server will not find the file and throw 404
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to rewrite the file path like the above, but failed, so what do you think here should to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rynclark , I tried to add this code back when testing with .ts
file, but there's a problem, here path.join(basePath, outputPath)
refers to the absolute path in the file system, so that 404, because .js
doesn't exist at this basePath
, so when replacing with path same aswebpackOptions.output.path = path.join(os.tmpdir(), '_karma_webpack_', indexPath, '/');
, still cannot find the compiled .js
file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move discussion about this into another PR and first of all fix the file path normalization issue as this seems to be critical?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried with v3.0.3, still needs this L393 changes which normalizes the path again. I have opened a PR #354 for 3.0. @michael-ciniawsky
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this PR is ready, with this changs, I have no issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sry 🙏 I skipped over https://github.com/webpack-contrib/karma-webpack/pull/351/files#r215968728
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np, thanks @michael-ciniawsky
Seeing similar error after upgrading to RC1:
|
src/karma-webpack.js
Outdated
@@ -308,7 +308,7 @@ Plugin.prototype.readFile = function(file, callback) { | |||
os.tmpdir(), | |||
'_karma_webpack_', | |||
String(idx), | |||
this.outputs.get(file) | |||
this.outputs.get(file.replace(/\\/g, '/')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this into it's own helper
const normalize = (file) => file.replace(/\\/g, '/')
@@ -383,9 +387,6 @@ function createPreprocesor(/* config.basePath */ basePath, webpackPlugin) { | |||
throw err; | |||
} | |||
|
|||
const outputPath = webpackPlugin.outputs.get(filename); | |||
file.path = path.join(basePath, outputPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move discussion about this into another PR and first of all fix the file path normalization issue as this seems to be critical?
windows
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Teamop 👍 Thx
@@ -383,9 +387,6 @@ function createPreprocesor(/* config.basePath */ basePath, webpackPlugin) { | |||
throw err; | |||
} | |||
|
|||
const outputPath = webpackPlugin.outputs.get(filename); | |||
file.path = path.join(basePath, outputPath); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Released in |
With #347 changes, now get error:
TypeError: Path must be a string. Received undefined
. After my research, there are 3 places will throw this error:Two in the
readFile
, L311, L337, both arethis.outputs.get(file)
--- the changes I made here is to map the file path to the entry path, so that can getting the file from the outputs. Example, changes from
test\unit\karma-test-shim
totest/unit/karma-test-shim
One in the
createPreprocesor
L386, this linewebpackPlugin.outputs.get(filename);
--- for this one, the file is already the real file path, if replaced with the webpack stats one, the karma web server cannot find the file 404.
Example here, changes from
C:/project/test/unit/karma-test-shim.js
toC:\project\test\unit\karma-test-shim.js
Type
Issues
SemVer