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

feat: use shared bundles #380

Merged
merged 1 commit into from
Dec 13, 2018

Conversation

daKmoR
Copy link
Collaborator

@daKmoR daKmoR commented Dec 9, 2018

This PR contains a:

  • new feature
  • code refactor
  • test update

Motivation / Use-Case

Refactor karma-webpack to create bundles not in isolation.
For the full story pls see this issue: #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'],
  1. 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:

  • update readme
  • cleanup dependencies (babel, async, ... no longer needed?)
  • make a proper git history once everything is done

Some initially optional tasks:

  • do proper testing (I will definitely follow up with an additional pull request if we say this is the way forward)

closes: #379

@jsf-clabot
Copy link

jsf-clabot commented Dec 9, 2018

CLA assistant check
All committers have signed the CLA.

@daKmoR daKmoR changed the base branch from master to 5.0.0 December 9, 2018 17:47
@matthieu-foucault matthieu-foucault changed the base branch from 5.0.0 to next December 9, 2018 18:54
@matthieu-foucault matthieu-foucault added this to the 5.0.0 milestone Dec 9, 2018
@matthieu-foucault
Copy link
Collaborator

@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
Copy link
Collaborator

Does this fix #22 as well?

@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 9, 2018

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
Copy link

dogoku commented Dec 10, 2018

@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
Copy link
Collaborator Author

daKmoR commented Dec 10, 2018

@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
Copy link

dogoku commented Dec 10, 2018

@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
Copy link

dogoku commented Dec 10, 2018

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
Copy link
Collaborator Author

daKmoR commented Dec 10, 2018

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

nice 👍 will change it later today 💪

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

awesome 🎉
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
Copy link

dogoku commented Dec 10, 2018

Alright, so a few more things:

Actual good news:

  • Mocha's describe.only works across files (makes sense since we are bundling correctly now).

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
Copy link
Collaborator

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
Copy link
Collaborator Author

daKmoR commented Dec 10, 2018

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 238744d

  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 8f08ba4

  1. 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 34b2342

  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?

  1. 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
Copy link
Collaborator Author

daKmoR commented Dec 10, 2018

@matthieu-foucault
there should be no vendors~spec.js but that should be fixed with 8f08ba4

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
Copy link
Collaborator

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
Copy link
Collaborator

how do you do the typescript processing?

With a webpack loader (awesome-typescript-loader)

@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 10, 2018

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
Copy link

dogoku commented Dec 11, 2018

@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
Copy link
Collaborator

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

@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 11, 2018

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 bf966f0 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
Copy link
Collaborator Author

daKmoR commented Dec 11, 2018

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

@matthieu-foucault via 5a05452 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
Copy link

dogoku commented Dec 11, 2018

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
Copy link
Collaborator Author

daKmoR commented Dec 12, 2018

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
Copy link

dogoku commented Dec 12, 2018

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
Copy link
Collaborator

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
Copy link
Collaborator

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 daKmoR force-pushed the feat/sharedBundles branch 2 times, most recently from 733dbe1 to 6c73423 Compare December 12, 2018 18:37
@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 12, 2018

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

README.md Outdated Show resolved Hide resolved
@matthieu-foucault
Copy link
Collaborator

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

BREAKING CHANGE: webpack needs to be added to frameworks

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

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

BREAKING CHANGE: old alternative usage is no longer recommended
BREAKING CHANGE: webpack-dev-middleware removed
BREAKING CHANGE: default webpack configuration changed drastically
@daKmoR
Copy link
Collaborator Author

daKmoR commented Dec 12, 2018

did a small Readme change

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

uh nice looking forward to it :)

@matthieu-foucault matthieu-foucault merged commit 2ab7ad5 into codymikol:next Dec 13, 2018
@dogoku
Copy link

dogoku commented Dec 13, 2018

thanks both for your hard work <3

@matthieu-foucault
Copy link
Collaborator

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
Copy link
Collaborator Author

daKmoR commented Dec 13, 2018

thx for the release :)

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

@thescientist13
Copy link

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
Copy link

jquense commented Aug 10, 2020

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants