nikku / karma-browserify

A fast Browserify integration for Karma that handles large projects with ease
MIT License
321 stars 50 forks source link

Fix: make karma-browserify compatible with parcelify #182

Closed mediafreakch closed 8 years ago

mediafreakch commented 8 years ago

I'd like to propose the following change in order to make karma-browserify compatible with parcelify. Parcelify needs an explicit entrypoint to concatenated the CSS in the right order. karma-browserify thus should use b.add instead of b.require to bundle the specs.

Unit tests still pass...

nikku commented 8 years ago

We could technically merge this (tests pass, as you said).

It would be a major version bump though because the change from require to add may break setups with older browserify versions.

The good news is that the following fails for plug-ins and transforms only:

cd karma-browserify
npm i browserify@10 watchify@3.2.x
grunt

I'll keep this PR open and merge it once other substantial changes force us to do a major release.

nikku commented 8 years ago

I still don't understand why parcelify cannot add support for required resources next to added ones.

mediafreakch commented 8 years ago

@nikku I cannot answer that but maybe @dgbeck can? I guess in the require case parcelify would just act as a concatenator for the css of required packages. Nothing wrong with that, but a contribution is beyond my expertise :(

mediafreakch commented 8 years ago

@nikku Do you have any suggestions for how to wait for the plugin to finish before karma launches the test?

dgbeck commented 8 years ago

Hi @nikku,

Thanks for your time regarding this tricky issue. Thankfully it sounds like the change in the PR will work, but I can answer the question about why parcelify can not support required resources...

The issue is that using require apparently does not imply that the file is an entry point, while using add does. If there is no entry point file, parcelify can not do its job properly, because the order in which it needs to concatenate the css files can not be determined in all cases. There are some hacky work arounds that we could employ to identify the entry point, but since it doesn't make sense to use parcelify without an entry point I'm not convinced that is fixing the problem at its root.

More context here https://github.com/rotundasoftware/parcelify/issues/39

Thx!

nikku commented 8 years ago

@dgbeck Thanks for clarifying.

I disagree with the following:

The issue is that using require [...] does not imply that the file is an entry point, while using add does.

Depending on how you use browserify using b.require does imply an entry point. People could use it like karma-browserify does by requiring the specified module during bootstrapping instead of automatically executing a particular part of the bundle (that is what b.add) does.

Karma serves a list of files and each of the files is a test stub that hooks up to a specific part of the bundle. May not be a use case for single-entry bundles but for more complicated setups.

nikku commented 8 years ago

I definitely see the issue with having too many entry points for generating CSS bundles, too.

@mediafreakch Maybe you should rethink the use-case of using karma-browserify with parcelify. Wouldn't generating the CSS file outside the bundle work, too?

mediafreakch commented 8 years ago

@nikku you mean as if manually running an npm script + checking in the compiled CSS into source control? (otherwise CI doesn't work). Yes of course it would work, but the whole point of a test runner is to automate, isn't it?

nikku commented 8 years ago

You can easily run that as a pre-step to your tests as part of CI:

npm run compile-css
npm test

No need for checking in stuff into source control.

mediafreakch commented 8 years ago

You are right. I'll try that. Thanks! I'll probably end up with something like:

{
    "scripts": {
           "test": "npm run compile-css && karma start"
    }
}
nikku commented 8 years ago

With the fix at hand I'll close this PR.