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(karma-webpack): handle multiple outputs correctly #357

Merged
merged 4 commits into from
Sep 14, 2018

Conversation

alabbas-ali
Copy link
Contributor

@alabbas-ali alabbas-ali commented Sep 14, 2018

When configuring Webpack to compile ts, the output is array and karma-webpack throw Exception like

08 09 2018 02:13:38.035:ERROR [karma]: 
TypeError: Path must be a string. Received [ 'spec/vendors.spec.js', 'spec/app.spec.js.map' ]
    at assertPath (path.js:28:11)
    at Object.join (path.js:1236:7)
    at Plugin.<anonymous> (../node_modules/karma-webpack/lib/karma-webpack.js:286:70)
    at Plugin.readFile (../node_modules/karma-webpack/lib/karma-webpack.js:305:5)
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickCallback (internal/process/next_tick.js:180:9)

Type

  • Fix

Issues

SemVer

  • Patch

@43081j
Copy link

43081j commented Sep 14, 2018

I do wonder if it'd make more sense to do the check when populating the outputs map.

If you can safely assume that the first entry is always the one we want, i would consider just adding the first one to the map and leave the existing code as is.

Are we sure that the first entry will always be right, too?

@alabbas-ali
Copy link
Contributor Author

I do wonder if it'd make more sense to do the check when populating the outputs map.

If you can safely assume that the first entry is always the one we want, I would consider just adding the first one to the map and leave the existing code as is.

Are we sure that the first entry will always be right, too?

the first entry is always the js file, the second entry may be the map file or CSS file etc.. depending on the configuration of webpack. but the first entry is always the chank JS file. I think this is what we want to test. If I am not mistaken

Copy link
Contributor

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

👍 One clearification and two nits

@@ -254,7 +258,11 @@ Plugin.prototype.readFile = function(file, callback) {
})
} else {
try {
var fileContents = middleware.fileSystem.readFileSync(path.join(os.tmpdir(), '_karma_webpack_', this.outputs[file]))
Copy link
Contributor

Choose a reason for hiding this comment

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

var fileContents = ''

if (condition) {
  fileContents = ...
} else {
  fileContents = ...
}

@@ -237,7 +237,11 @@ Plugin.prototype.readFile = function(file, callback) {
var doRead = function() {
if (optionsCount > 1) {
async.times(optionsCount, function(idx, callback) {
middleware.fileSystem.readFile(path.join(os.tmpdir(), '_karma_webpack_', String(idx), this.outputs[file]), callback)
if (Array.isArray(this.outputs[file])) {
middleware.fileSystem.readFile(path.join(os.tmpdir(), '_karma_webpack_', String(idx), this.outputs[file][0]), callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ensured that this.outputs[file][0] points to the 'correct' file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you try it it with e.g source maps enabled and/or extracting CSS ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it with both cases, it works very well :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright :), I need to rely upon you here since this module has no test 😥

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, It seems that it is not always the case for some webpack config :(
So i had to fix this in #360

middleware.fileSystem.readFile(path.join(os.tmpdir(), '_karma_webpack_', String(idx), this.outputs[file]), callback)
if (Array.isArray(this.outputs[file])) {
middleware.fileSystem.readFile(path.join(os.tmpdir(), '_karma_webpack_', String(idx), this.outputs[file][0]), callback);
}else{
Copy link
Contributor

Choose a reason for hiding this comment

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

- }else{
+ } else { 

@michael-ciniawsky michael-ciniawsky added this to the 3.0.5 milestone Sep 14, 2018
@michael-ciniawsky michael-ciniawsky changed the title 🐛 fix TypeError: Path must be a string. Received Array fix(karma-webpack): handle multiple outputs correctly Sep 14, 2018
@@ -254,7 +258,13 @@ Plugin.prototype.readFile = function(file, callback) {
})
} else {
try {
var fileContents = middleware.fileSystem.readFileSync(path.join(os.tmpdir(), '_karma_webpack_', this.outputs[file]))

var fileContents = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent seems to be off here :)

@michael-ciniawsky
Copy link
Contributor

@alabbas-ali Would you mind sending a PR with these changes formaster (v4.0.0-rc.2) aswell ? 🙏


var fileContents = ''
try {
var fileContents = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Still off 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe my editor is off 🔢

Copy link
Contributor

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

@michael-ciniawsky michael-ciniawsky merged commit 59de62c into codymikol:3.0.0 Sep 14, 2018
@michael-ciniawsky
Copy link
Contributor

Released in v3.0.5 🎉

@michael-ciniawsky michael-ciniawsky removed this from the 3.0.5 milestone Sep 14, 2018
@alabbas-ali alabbas-ali deleted the multiple-output-files branch September 14, 2018 14:43
@alabbas-ali
Copy link
Contributor Author

I do wonder if it'd make more sense to do the check when populating the outputs map.
If you can safely assume that the first entry is always the one we want, I would consider just adding the first one to the map and leave the existing code as is.
Are we sure that the first entry will always be right, too?

the first entry is always the js file, the second entry may be the map file or CSS file etc.. depending on the configuration of webpack. but the first entry is always the chank JS file. I think this is what we want to test. If I am not mistaken

I did more testing, with different config, unfortunately, I found out that the assumption "that the first file is the JS" is not the case always, So I had to fix this please check out #360

@edmorley
Copy link

Could we also uplift this to the 4.x branch too?

@edmorley
Copy link

Could we also uplift this to the 4.x branch too?

Ping?

This is blocking us updating to 4.x, since otherwise we get:

21 09 2018 23:54:23.585:ERROR [karma]: TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received type object

@alabbas-ali
Copy link
Contributor Author

alabbas-ali commented Sep 21, 2018

Could we also uplift this to the 4.x branch too?

Ping?

This is blocking us updating to 4.x, since otherwise we get:

21 09 2018 23:54:23.585:ERROR [karma]: TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received type object

I create a pull request out of it #361. but please review #360 before.

@fulv
Copy link

fulv commented Sep 28, 2018

Is this going to be included in the next 4.x release?

@thijstriemstra
Copy link

Can this be released?

@fulv
Copy link

fulv commented Nov 30, 2018

Thank you, it works for me now! +1

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.

6 participants