jamesshore / quixote

CSS unit and integration testing
Other
848 stars 45 forks source link

change main script to src directory #37

Closed JuanCaicedo closed 7 years ago

JuanCaicedo commented 7 years ago

I was having some trouble using browserify to build my karma tests. I saw that in the example code both browserify and commonjs are. This change makes it possible to just use browserify. You can view some working code using this fork here. To test it:

$ git clone https://github.com/JuanCaicedo/tdd-css.git
$ cd tdd-css
$ npm install
$ npm test

I think main is only used by node, when determining what to use as an entry point, but if you're concerned there might be breaking changes then I can try to come up with some test code.

A potential downsite is that using dist as an entry point means that you can have a build step before. That is, you could for example transpile first and have node use the code after it has been transpiled, instead of needing to somehow do it at runtime. This might become a concern in the future

jamesshore commented 7 years ago

What is the exact issue you're seeing? dist should work fine for any tool that supports CommonJS Modules, including Browserify.

JuanCaicedo commented 7 years ago

@jamesshore I've pushed a new branch to that test repo called quixote-published which uses quixote installed from npm. If you run $ npm test you should see errors along the lines of

01 02 2017 15:32:24.359:ERROR [framework.browserify]: bundle error
01 02 2017 15:32:24.359:ERROR [framework.browserify]: Error: Cannot find module './util/oop.js' from '/Users/JuanCaicedo/code/speaking/tdd-css-workshop/node_modules/quixote/dist'
jamesshore commented 7 years ago

I tried your repo and saw the error you're referring to. I'm not sure why your Karma build is failling, but dist/quixote.js is a self-contained module that has all dependencies included. It should work without any issues. This looks like an issue with the browserify plugin.

JuanCaicedo commented 7 years ago

I referenced this in the above issue for the karma-browserify project

jamesshore commented 7 years ago

I'll go ahead and leave this open for now. Let me know what the karma-browserify people say.

JuanCaicedo commented 7 years ago

@jamesshore I asked and got this feedback from them https://github.com/nikku/karma-browserify/issues/204#issuecomment-277461094

A short summary is that they asked me to run browserify manually to see if the build worked. It didn't. They said it's probably that quixote is using require internally and that confuses browserify.

One thing that's interesting is that with the changes in this fork, which I've pushed here, I no longer get a build time error, but now I get one at runtime:

$ browserify test/accent-box.js -o lib-test/accent-box.js -t brfs -t [ babelify --presets [ es2015 ] ]
$ npm test
> tdd-css@1.0.0 test /Users/JuanCaicedo/code/speaking/quixote-browserify
> karma start --single-run

module.js:327
    throw err;
    ^

Error: Cannot find module '../lib/cli'
    at Function.Module._resolveFilename (module.js:325:15)
    at Function.Module._load (module.js:276:25)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (/Users/JuanCaicedo/code/speaking/quixote-browserify/node_modules/.bin/karma:3:1)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Function.Module.runMain (module.js:441:10)
npm ERR! Test failed.  See above for more details.
JuanCaicedo commented 7 years ago

Perhaps the problem is something in how quixote is being built?

jamesshore commented 7 years ago

Sorry for the slow response. I've reproduced the problem with manual browserify and confirmed that changing the package.json's main to src fixes it.

I'm going to dig into the way quixote's being built to see if there's a solution there, but if not, your patch should also take care of it.

jamesshore commented 7 years ago

Okay, it looks like you can't use browserify on browserify'd files. I think your solution is the correct one. Thanks again!

jamesshore commented 7 years ago

Released fix in v0.12.4.