codymikol / karma-webpack

Karma webpack Middleware
MIT License
830 stars 222 forks source link

feat: use shared bundles #380

Closed daKmoR closed 5 years ago

daKmoR commented 5 years ago

This PR contains a:

Motivation / Use-Case

Refactor karma-webpack to create bundles not in isolation. For the full story pls see this issue: https://github.com/webpack-contrib/karma-webpack/issues/379

Breaking Changes

1) As this now uses a karma framework to add additional files this will need to be added everywhere.

// old:
frameworks: ['mocha'],

// new:
frameworks: ['mocha', 'webpack'],

2) The Alternative Usage described in the readme would be no longer needed.

Additional Info

I am aware that this is a huge refactor and while working on this feature I sort of already expected that it will end up with something like this. Again pls check the issue for how we ended up here.

This also removed the build step (don't think it's needed anymore) so we can easily test it as follows.

Howto test

easiest way:

# replace karma-webpack with forked version
npm i daKmoR/karma-webpack#feat/sharedBundles
# invoke as usual probably like this
npm run test

Possible ways to move forward

I see still some mandatory tasks:

Some initially optional tasks:

closes: https://github.com/webpack-contrib/karma-webpack/issues/379

jsf-clabot commented 5 years ago

CLA assistant check
All committers have signed the CLA.

matthieu-foucault commented 5 years ago

@daKmoR FYI, this:

npm i karma-webpack
# replace karma-webpack with forked version
cd node_modules
rm -fr karma-webpack
git clone https://github.com/daKmoR/karma-webpack.git
cd karma-webpack
git checkout feat/sharedBundles
npm i

can be replaced by npm i daKmoR/karma-webpack#feat/sharedBundles

matthieu-foucault commented 5 years ago

Does this fix #22 as well?

daKmoR commented 5 years ago

npm i daKmoR/karma-webpack#feat/sharedBundles

uh nice I didn't knew :)

Does this fix #22 as well?

yes, CommonsChunkPlugin got "replaced/renamed" to splitChunks which is used in this implementation

dogoku commented 5 years ago

@daKmoR I'm beginning to test your branch out on our projects. I'll be commenting as I find issues.

Right off the bat, using Mac (Sierra), I had to create _karma_webpack_ in the temp folder myself, as karma would fail immediately and not run at all.

10 12 2018 09:23:08.289:ERROR [karma-server]: Server start failed on port 9876: Error: ENOENT: no such file or directory, open '/var/folders/1f/kdnyqc3n2t7f8l8kxcj0wk5w5t9pqt/T/_karma_webpack_/commons.js'

Once I did that, karma started running, but it seems like it's not respecting some of our custom path resolves, failing most of our tests. Still investigating if that's something caused by our setup as we are running karma through our own custom cli app, instead of directly from a project (think vue-cli), so it could be our fault.

daKmoR commented 5 years ago

@dogoku thx for testing it :)

I hope fix: make sure tmp folder exists solves the temp folder issue.

Also I didn't deep merge custom options yet - so every option you set on karmaconfig.webpack was not applied yet. That should work after fix: allow for deep nested webpack options.

=> this adds a dependency to merge - it was the first thing I found - if we have a different preference which lib to use for deep left merging just let me know.

dogoku commented 5 years ago

@daKmoR thanks for the update, I'll give it a go and let you know.

For merging configs, I'd suggest using https://www.npmjs.com/package/webpack-merge

It's a smarter version of merge that handles functions in a much better way. If however you feel that's an overkill, a deep merge like the one you are using might be enough as well

dogoku commented 5 years ago

I'm trying your latest change, but now webpack is complaining that the configuration object is not valid, in regards to the plugins.

image

I suspect what's happening is that the merge utility you are using is not handling the plugins correctly. If possible, give webpack-merge mentioned above a try, which should handle plugins better.


Update 1: Using webpack-merge, I was able to proceed to the next step:

  updateWebpackOptions(newOptions) {
    // this.__webpackOptions = merge.recursive(
    //   true,
    //   this.__webpackOptions,
    //   newOptions
    // );
    this.__webpackOptions = webpackMerge(
      this.__webpackOptions,
      newOptions
    );
  }

I can now see everything compiling (and more importantly splitting), but Karma thinks that there are no tests to run. Will continue debugging and let you know.


Update 2: Finally, some good news :D

The issue of no tests being detected, was my code's issue. So with that aside, I am successfully running and passing over 80% of my tests.

daKmoR commented 5 years ago

Update 1: Using webpack-merge, I was able to proceed to the next step:

nice :+1: will change it later today :muscle:

I am successfully running and passing over 80% of my tests.

awesome :tada:
if you find anything else that needs change just let me know :) also I am curious do you notice any difference in speed/performance?

dogoku commented 5 years ago

Alright, so a few more things:

Actual good news:

Potential good news:

Actual issues:

1) Passing a pattern that matches a single file to config.preprocessors, will cause karma to not recognise the tests (this is the issue I had above). E.g:

{
    files: [
      { pattern: 'test/unit/services/user-service/user-service-test.js' }
    ],
    preprocessors: {
      'test/unit/services/user-service/user-service-test.js': ['webpack']
    },
}

At the very least 2 files need to be matched by the patterns passed to preprocessor. E.g

{
    files: [
      { pattern: 'test/unit/services/user-service/user-service-test.js' }
    ],
    preprocessors: {
      // A glob that matches atleast 2 files
      'test/unit/**/*-test.js': ['webpack'],

      // OR 2 individual entries
      'test/unit/services/user-service/user-service-test.js': ['webpack'],
      'test/unit/services/data-service/data-service-test.js': ['webpack']
    },
}

2) All files matched by the patterns in config.preprocessors are bundled by webpack, regardless if we are not specifying them in config.files

From the 2nd example above, 'test/unit/**/*-test.js': ['webpack'] will cause webpack to create an entry for all my unit tests, even though I am only testing 1 file with karma { pattern: 'test/unit/services/user-service/user-service-test.js' }

Perhaps, we can cross-reference config.files and config.preprocessors to avoid creating entries for files that are not going to be used in the current test run. I imagine this is easier said than done, given Karma's file config is quite complex (http://karma-runner.github.io/3.0/config/files.html).

Potential issues

1) Watch mode was not waiting for all plugins to finish work and resulted in webpack compiling multiple times whenever I saved

This one is hard to explain, I need to spend more time debugging our custom plugins to make sure it's not them, before I can give you a clear case to work against.

2) Bundle sizes seem bigger than what they should, (some chunks are over 2MB)

I'm guessing it's babel-polyfill adding polyfills to each bundle, but again I need to confirm this one.


That's all for now. I'll update this post, as I have more details on each problem.

matthieu-foucault commented 5 years ago

Having an issue with a Typescript project.

The bug

In processFile(content, file, done) (karma-webpack.js:68), an exception is thrown because bundleContent is undefined.

The cause

path.parse(file.path).base would return spec.ts, while the keys of bundleContent are [ 'runtime.js', 'spec.js', 'vendors~spec.js' ]

Solution

It seems that using path.parse(file.path).name + '.js' fixes that part. Still having some errors further down the line

daKmoR commented 5 years ago

First, thank you for this awesome test feedback πŸ’ͺ
makes it really easy for me to pinpoint the problem and fix it πŸ‘

Update 1: Using webpack-merge, I was able to proceed to the next step:

Use it via via https://github.com/webpack-contrib/karma-webpack/pull/380/commits/238744ddd36ec671a50852a7b6944fc3b8fb773d

1) Passing a pattern that matches a single file to config.preprocessors, will cause karma to not recognise the tests (this is the issue I had above). E.g:

fixed via https://github.com/webpack-contrib/karma-webpack/pull/380/commits/8f08ba4e7e4b62ce0f2ca9ec182163e9f8601bdd

2) All files matched by the patterns in config.preprocessors are bundled by webpack, regardless if we are not specifying them in config.files

yeah seems I was a little eager with that one... turned the logic around... get all files first and then filter by preprocessor patterns via https://github.com/webpack-contrib/karma-webpack/pull/380/commits/34b23425cd04e88af80db15543d979452c771840

1) Watch mode was not waiting for all plugins to finish work and resulted in webpack compiling multiple times whenever I saved

which plugins are we talking about here? other karma preprocessors? or webpack plugins?

2) Bundle sizes seem bigger than what they should, (some chunks are over 2MB)

that seems weird - all the common parts (incl. babel polyfill should be within the commons.js) so commons.js should be big but each test file should only contain stuff unique to it

daKmoR commented 5 years ago

@matthieu-foucault there should be no vendors~spec.js but that should be fixed with https://github.com/webpack-contrib/karma-webpack/commit/8f08ba4e7e4b62ce0f2ca9ec182163e9f8601bdd

path.parse(file.path).base would return spec.ts

that could be "problematic" how do you do the typescript processing? is it a karma preprocessor or a separate build process?

matthieu-foucault commented 5 years ago

Seeing these warnings down the line: [middleware:karma]: Invalid file type, defaulting to js. ts The browser refuses to execute the tests because of a mime type issue. I'll investigate later, and make a small typescript repo for easy testing.

matthieu-foucault commented 5 years ago

how do you do the typescript processing?

With a webpack loader (awesome-typescript-loader)

daKmoR commented 5 years ago

I'll investigate later, and make a small typescript repo for easy testing.

yes pls :) I don't have a typescript project using karma at hand πŸ™ˆ

dogoku commented 5 years ago

@daKmoR the latest version fixed both problems I mentioned above, build seems considerably faster now, and since very few chunks are generated πŸ‘

I am now trying to make the output of webpack less verbose, but so far it's ignoring any stats: 'errors-only' i pass to it.

matthieu-foucault commented 5 years ago

@daKmoR : here's a minimal example allowing you to test the issue with Typescript: https://github.com/matthieu-foucault/ts-karma-webpack-test

daKmoR commented 5 years ago

I am now trying to make the output of webpack less verbose, but so far it's ignoring any stats: 'errors-only' i pass to it.

via https://github.com/webpack-contrib/karma-webpack/pull/380/commits/bf966f0e1d0c78ade28a312a05ee60d6df1c8e17 it's now mimicking the webpack-cli for stats so you can configure whatever you want πŸ‘

What do you think would be good defaults? that's it now

  stats: {
    modules: false,
    colors: true,
  },

@daKmoR : here's a minimal example allowing you to test the issue with Typescript:

thx will take a look πŸ€—

daKmoR commented 5 years ago

@daKmoR : here's a minimal example allowing you to test the issue with Typescript:

@matthieu-foucault via https://github.com/webpack-contrib/karma-webpack/pull/380/commits/5a05452e34638270fdf88f6dddbf13140e900535 it now forces *.js files by default and also allows you to add your own via config.webpack.transformPath

solution inspired by karma-coffee-preprocessor

dogoku commented 5 years ago

it's now mimicking the webpack-cli for stats so you can configure whatever you want πŸ‘

What do you think would be good defaults?

Personally, I like to turn off logging during tests, using stats: 'errors-only'. (dots is my fav reporter) As long as we have the ability to configure, I think your suggested defaults are good enough.

daKmoR commented 5 years ago

Did we now reach a point where it makes sense to update this PR to become merge worthy (e.g. update Readme, clean up history, anything else?)

dogoku commented 5 years ago

I'd say it's a good enough beta.

We have managed to make all our tests green (the ones failing where not cleaning up properly).

Our stack includes: mocha, chai, chai-as-promised, sinon-chai, rewiremock (which requires a webpack plugin), custom webpack plugins (for localisation, config injection, etc). Being able to run all of that means it should handle most things we throw at it.

There's still a couple of things I want to test when in debug (watch) mode, but I have been busy with a different issue.

The question becomes, when can we release this, given v4 is still in RC and given the Typescript rewrite initiative

matthieu-foucault commented 5 years ago

I'd say it's a good enough beta.

I agree, not an RC yet

The question becomes, when can we release this, given v4 is still in RC and given the Typescript rewrite initiative

Whenever you want, it's a beta. v4 still has some bugs I would like to fix (#100, #337, #272), but I have no issue with v5 moving forward before v4 is released

matthieu-foucault commented 5 years ago

e.g. update Readme, clean up history, anything else

It will all be squashed into one commit, so don't worry about cleaning up history. You'll need to undo the version change in package.json. standard-version will bump that for us, as well as write the changelog.

daKmoR commented 5 years ago

It will all be squashed into one commit, so don't worry about cleaning up history.

I feel more comfortable doing it myself πŸ™ˆ Added also all needed info for changelog to commit message

You'll need to undo the version change in package.json.

did that and updated readme - anything else?

=> if not it would be really nice to have a 5.0.0 alpha/beta/... release

matthieu-foucault commented 5 years ago

Looks good. I'll make an alpha release later today

daKmoR commented 5 years ago

did a small Readme change

Looks good. I'll make an alpha release later today

uh nice looking forward to it :)

dogoku commented 5 years ago

thanks both for your hard work <3

matthieu-foucault commented 5 years ago

Alright! It's available under the dev tag, so npm i -D karma-webpack@dev should do it. Hopefully I can release v4 this week, and after that I think we should go with a blank slate, i.e. delete all old issues and focus on making v5 the best version of this project πŸ˜„

daKmoR commented 5 years ago

thx for the release :)

and that sound like a very awesome move forward :) count me in :)

thescientist13 commented 5 years ago

Just wanted to stop by and say thank you for this work @daKmoR ! Was getting the same issue for testing web components and using the @dev version of this plugin fixed the issue. Thank you to you and the karma-webpack team! πŸ™Œ

jquense commented 4 years ago

greetings from 2020, does this approach work well from folks who have tried it? Wondering if it's better to switch to the forever-beta

theo-staizen commented 4 years ago

Yes, it has been working for us without any issues for a while now. It's been so long I forgot it was in beta still... Having said that, we recently switched to a new stack, that doesnt involve karma, so I dont know whether this will still work for the upcoming webpack 5