search

codymikol / karma-webpack

Karma webpack Middleware
MIT License
829 stars 222 forks source link

Fix getPathKey for TypeScript files #451

Closed appzuka closed 3 years ago

appzuka commented 3 years ago

This PR contains a:

Motivation / Use-Case

I was seeing the same problem reported by @thijstriemstra:

https://github.com/ryanclark/karma-webpack/issues/450#issuecomment-707961142

I am using TypeScript and the cause seems to be that getPathKey uses '.ts. to generate the hash for the file contents when creating the bundle but tries to use '.js' when reading it back. As the key is different, the bundle is not found, causing the error. This patch fixes this for .ts files.

Breaking Changes

Additional Info

appzuka commented 3 years ago

Updated fix so that it works for both ts and tsx files. Also, moved patch into configToWebpackEntries because in processFile the filename is already transformed. Tested with .js, .ts and .tsx files.

thijstriemstra commented 3 years ago

thanks for the pr @appzuka

I installed your pr using npm install -D appzuka/karma-webpack#next and ran the tests but nothing seems to be executed:

Webpack bundling...
asset commons.js 943 KiB [emitted] [big] (name: commons) (id hint: commons) 1 related asset
asset runtime.js 5.34 KiB [emitted] (name: runtime) 1 related asset
asset mediaelement-webaudio.spec.2732148482.js 746 bytes [emitted] (name: mediaelement-webaudio.spec.2732148482) 1 related asset
asset mediaelement.spec.2650037284.js 719 bytes [emitted] (name: mediaelement.spec.2650037284) 1 related asset
asset plugin-api.spec.2330068884.js 713 bytes [emitted] (name: plugin-api.spec.2330068884) 1 related asset
asset wavesurfer.spec.1154267014.js 713 bytes [emitted] (name: wavesurfer.spec.1154267014) 1 related asset
asset peakcache.spec.320896041.js 708 bytes [emitted] (name: peakcache.spec.320896041) 1 related asset
asset drawer.spec.3087432955.js 701 bytes [emitted] (name: drawer.spec.3087432955) 1 related asset
asset util.spec.3953596729.js 695 bytes [emitted] (name: util.spec.3953596729) 1 related asset
asset wavesurfer.js 675 bytes [emitted] (name: wavesurfer) 1 related asset
webpack 5.2.0 compiled successfully in 5103 ms
23 10 2020 21:32:50.652:INFO [karma-server]: Karma v5.2.3 server started at http://localhost:9876/
23 10 2020 21:32:50.653:INFO [launcher]: Launching browsers Chrome_dev, Firefox_dev with concurrency unlimited
23 10 2020 21:32:50.659:INFO [launcher]: Starting browser Chrome
23 10 2020 21:32:50.887:INFO [launcher]: Starting browser Firefox
23 10 2020 21:32:53.901:INFO [Firefox 81.0 (Mac OS 10.15)]: Connected on socket 9SFLrHi6dJFRVOy5AAAA with id 30068456
23 10 2020 21:32:54.509:INFO [Firefox 81.0 (Mac OS 10.15)]: Starting tests 30068456
Firefox 81.0 (Mac OS 10.15): Executed 0 of 0 SUCCESS (0.012 secs / 0 secs)
23 10 2020 21:32:56.363:INFO [Chrome 86.0.4240.111 (Mac OS 10.15.7)]: Connected on socket P68_Yphh-FkQbcL0AAAB with id 42575374
Firefox 81.0 (Mac OS 10.15): Executed 0 of 0 SUCCESS (0.012 secs / 0 secs)
Chrome 86.0.4240.111 (Mac OS 10.15.7): Executed 0 of 0 SUCCESS (0.001 secs / 0 secs)

Suites and tests results:

Browser results:

TOTAL: 0 SUCCESS
appzuka commented 3 years ago

Thanks for taking a look. I have created a minimal repository with 3 tests that demonstrates that the existing branch for next will not handle files with an extension other than .js.

https://github.com/appzuka/karma-webpackv5-test

When you switch to my PR the tests all pass. The only difference in my PR is the following line:

webpackEntries[getPathKey(filePath.replace(/\.tsx?$/, '.js'))] = filePath;

I have added the .replace(/.tsx?$/, '.js'). If you are using js files it should be the same. If you can share some code where it does not work I'd be happy to take a look.

thijstriemstra commented 3 years ago

thanks for the feedback. This is the config in the branch I'm working on: https://github.com/katspaugh/wavesurfer.js/blob/webpack5/karma.conf.js

https://github.com/katspaugh/wavesurfer.js/tree/webpack5/build-config contains the webpack configs.

appzuka commented 3 years ago

In karma-webpack v5 you need to specify webpack in the list of frameworks:

    frameworks: ['jasmine', 'jasmine-matchers', 'webpack'],

With this, your tests run. This is mentioned in the Readme for the next branch but it is not easy to spot. I assume a list of breaking changes will be given when v5 is released.

Note that you don't need my PR as you are only using js files.

thijstriemstra commented 3 years ago

thanks @appzuka did fixed it. But now there's an issue with webpack versioning in your branch, see https://travis-ci.org/github/katspaugh/wavesurfer.js/builds/738634954

can you also bump the webpack version (or should it be backwards compatible with webpack 4)?

thijstriemstra commented 3 years ago

That last change didn't make a difference, I had to specify npm install --legacy-peer-deps in travis for it to work.

Note that you don't need my PR as you are only using js files.

Well, without your PR (and karma-webpack@5.0.0-alpha.3.0) I get this error, so it is needed IMO.

25 10 2020 12:26:13.559:ERROR [config]: Error in config file!
  Error: Cannot find module 'webpack'
Require stack:
- /home/thijs/projects/wavesurfer.js/build-config/fragments/common.js
- /home/thijs/projects/wavesurfer.js/build-config/webpack.prod.main.js
- /home/thijs/projects/wavesurfer.js/karma.conf.js
- /home/thijs/projects/wavesurfer.js/node_modules/karma/lib/config.js
- /home/thijs/projects/wavesurfer.js/node_modules/karma/lib/server.js
- /home/thijs/projects/wavesurfer.js/node_modules/karma/lib/cli.js
- /home/thijs/projects/wavesurfer.js/node_modules/karma/bin/karma
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:880:15)
    at Function.Module._load (internal/modules/cjs/loader.js:725:27)
    at Module.require (internal/modules/cjs/loader.js:952:19)
    at require (internal/modules/cjs/helpers.js:88:18)
    at Object.<anonymous> (/home/thijs/projects/wavesurfer.js/build-config/fragments/common.js:4:17)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Module._compile (/home/thijs/projects/wavesurfer.js/node_modules/pirates/lib/index.js:99:24)
    at Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Object.newLoader [as .js] (/home/thijs/projects/wavesurfer.js/node_modules/pirates/lib/index.js:104:7)
    at Module.load (internal/modules/cjs/loader.js:928:32)
appzuka commented 3 years ago

Hi @thijstriemstra,

My reason for using karma-webpackv5.0.0 is that I have upgraded to webpack 5 and I found I could not use karma-webpack v4. I then found that I could not use v5.0.0 without a patch because I am using typescript files. (I since found that @naoak created a patch for the same issue earlier: https://github.com/ryanclark/karma-webpack/issues/423#issuecomment-616274874).

I have not been deeply involved with karma-webpack but from the discussion, it seems to me that v5.0.0 is not only to make karma-webpack compatible with webpackv5 although it is necessary if you do want to upgrade webpack.

I have updated my branch to allow webpack 4 or 5 as a dependency although I have only tested this with webpack 5. I will leave it to the maintainers of karma-webpack to say whether this is correct.

If you are using karma-webpackv5.0.0 you do need to update karma.conf.js to include frameworks: ['jasmine', 'jasmine-matchers', 'webpack'] but that is not part of my PR.

The only difference between my repository and the next branch of https://github.com/ryanclark/karma-webpack concerns .ts and .tsx files so I would be surprised if you see any difference. My PR only addresses the issue of .ts files so if you are seeing further errors and/or version conflicts I suggest you raise them as a separate issue.

vicneanschi commented 3 years ago

@appzuka thanks for the PR! I'm trying to migrate to Webpack 5 and your changes fix the issue with TS. Could this be merged?

appzuka commented 3 years ago

I'm not sure if karma-webpack is still active. There have been no new commits into the master or next branches for over a year. I don't have write access so cannot merge this. Perhaps there is another project that achieves the same thing as this or fork of this one which is being updated, but I don't know of any.

himdel commented 3 years ago

Also dealing with a webpack5 upgrade, what worked for me is dropping karma-webpack entirely, and just running webpack before karma.

Essentially,


     files: [
-      {pattern: './client/app.js'},
+      {pattern: './dist/app.js'},
...
     preprocessors: {
-      './client/app.js': ['webpack']
codymikol commented 3 years ago

Hey @appzuka I'm going to take a look at this over this coming week. In the meantime, you can create a root .js file specifically for testing that imports all necessary ts and js files. This is what I'm using on one of my projects. I'll see if the underlying hashing issue is something that can be fixed though.

thijstriemstra commented 3 years ago

Well, without your PR (and karma-webpack@5.0.0-alpha.3.0) I get this error, so it is needed IMO.

fyi, all issues with karma-webpack were resolved for me with 5.0.0-alpha6.

appzuka commented 3 years ago

I haven't been keeping a close track on the recent changes to karma-webpack but I saw that v5 is now released. I tried it with a small repo using typescript and it worked, so I guess this issue is fixed in v5 and this PR is no longer required.

codymikol commented 3 years ago

Thanks for taking a look at this!