baconjs / bacon.jquery

MIT License
74 stars 23 forks source link

Refactor Gruntfile to dev vs. dist builds #12

Closed joefiorini closed 11 years ago

joefiorini commented 11 years ago

This sets the stage for better automated testing and consistency across browser vs. CLI tests. I'm opening this PR now but I'm still planning on pushing more changes to it. Presently I'm setting up Browserify to run tests in the browser; following that we'll be able to get the tests running in different browsers. I'm hoping we can use SauceLabs or something similar that provides free testing for open source projects.

For the changes thus far, we can build just like before with a new command: grunt build:dist, but I've also provided a dev build that builds to tmp so we can develop without putting the changes in dist.

Work Remaining

What do you think of this plan? I'm open to a simpler path if you have any other ideas.

raimohanska commented 11 years ago

Sounds very good!

Just bear in mind that these features are not to be compromised:

Won't be a problem probably though. I'd say these are the criteria for merging into master.

We might consider porting the good things to core Bacon.js too, when done here.

joefiorini commented 11 years ago

A quick update:

I've got the browserify build working, but the browserify example breaks (I haven't tested the others yet). I think my lack of hardcore experience with browserify is throwing me for a loop right now. I'm trying to load in the dist version of Bacon.JQuery.Bindings.js instead of the npm one, so we can always see the latest build working against the example without having to publish to npm. The example loads, but I get an error calling asTextValue from bundle.js; it seems that $ in that context is different than in bjq, therefore doesn't have asEventStream on its prototype. Will look into it more soon; I'm going to push what I have so far; any ideas here @raimohanska?

joefiorini commented 11 years ago

I have the browserify example working. In the other two it can't find Bacon. I think I need to shim it in the grunt config. I'll try that later and see what happens.

joefiorini commented 11 years ago

@raimohanska Turns out that browserify isn't going to work for us (see substack/node-browserify#462). In fact, I think browserify is too complicated for what we're trying to do. Instead, I implemented ES6 modules to compile using the "globals" configuration. This gives us a very nice syntax for importing, lets other users load their own dependencies and will make switching to node easy as well.

Note, I also updated the plan in the original message on this PR. Now that I finally have a sane build, I'm going to make sure all the examples still work then focus on the tests. Sound good?

raimohanska commented 11 years ago

I'm not 100% following (would need some time to focus), but sounds like you're doing good work. If the examples work (relatively) unchanged and the other aforementioned requirements are met, it's probably a good way to go. I'll have to check this out more closely soon.

joefiorini commented 11 years ago

Once I get the requirejs example working I'll push it and you'll be able to see the few changes I needed to make. Then we can decide if they're acceptable or if we should do a little more work to get a UMD wrapper in place.

joefiorini commented 11 years ago

Got a consistent build working that wraps the code in a UMD definition to work with CommonJS, AMD or neither. All 3 examples work great and I'm happy with the configuration needed. The only thing I'm not happy with is having to surround imports/exports with back-ticks, but the module transpiler doesn't support CoffeeScript anymore. Hopefully CS will eventually add some ES6 support and this won't be necessary.

@raimohanska What do you think of this setup?

joefiorini commented 11 years ago

Phew... I hadn't updated from master since I originally started on this (oops). Apparently when the Gruntfile changes from JavaScript to CoffeeScript it causes a lot of conflicts. Who knew? :smile: It's now as if it never happened. In fact, I probably could have not even mentioned my mistake. Oh well.

joefiorini commented 11 years ago

All tests passing, both unit tests and browser tests. I made a few important changes here:

There are a few commands for running the tests:

This opens the door in the near future to get the browser tests running in CI builds via Travis using Sauce Labs. Are you interested in setting something like that up?

joefiorini commented 11 years ago

Bonus! I just found grunt-release that does exactly the same thing as the release script currently in the repo. Set it up and it looks like it will do everything we need it to (obviously, I couldn't push to npm). There's currently an outstanding PR to allow it to update both bower.json and package.json. I'm using that fork at the moment.

raimohanska commented 11 years ago

Sounds good. Do you think this is ready for a merge?

joefiorini commented 11 years ago

If you're good with it, I don't see why not. Do you want me to open issues to cover the remaining tasks?

joefiorini commented 11 years ago

I went ahead and created an issue for the task of splitting the library out into multiple files. Since SauceLabs requires a signup, I'm leaving that one up to you. This is ready for you whenever.

raimohanska commented 11 years ago

Trying to build:

Loading "Gruntfile.coffee" tasks...ERROR
>> Error: Cannot find module './tasks/options/release'
Warning: Task "build" not found. Use --force to continue.

Also wondering why not still have npm install generate the dist files and npm test run tests? Now they don't seem to do anything except fetching deps.

joefiorini commented 11 years ago

Oops, added that file. I'll add the hooks for npm install & test when I get some time later.

On Wed, Aug 7, 2013 at 10:14 AM, Juha Paananen notifications@github.comwrote:

Trying to build:

Loading "Gruntfile.coffee" tasks...ERROR

Error: Cannot find module './tasks/options/release' Warning: Task "build" not found. Use --force to continue.

Also wondering why not still have npm install generate the dist files and npm test run tests? Now they don't seem to do anything except fetching deps.

— Reply to this email directly or view it on GitHubhttps://github.com/raimohanska/bacon-jquery-bindings/pull/12#issuecomment-22254000 .

raimohanska commented 11 years ago

I renamed and move the stuff. Now some conflicts need to be solved before getting to merge this. Sorry!

joefiorini commented 11 years ago

No problem. It will take a little bit of time. I'll hopefully get to it tomorrow morning.

joefiorini commented 11 years ago

@raimohanska All done, ready to merge?

raimohanska commented 11 years ago

We certainly need updated instructions for setting up, building and testing

raimohanska commented 11 years ago

Seem that setup involves

npm install -g grunt-cli phantomjs
npm install

Building the dist files is

grunt build:dist

Testing is

grunt test
raimohanska commented 11 years ago

Issues:

joefiorini commented 11 years ago

I will update the README with instructions, thanks for catching that. I can add grunt-cli and phantomjs to package.json. I'll take care of the issues you listed soon. Re: the browser tests, what platform are you running on?

Thanks man!

joefiorini commented 11 years ago

Just a quick update, had to switch gears for another project. Wrapping it this weekend, will finish this up early next week.

joefiorini commented 11 years ago

I've updated the NPM scripts to be able to run npm test and to run grunt build:dist after an install. grunt-cli will need to be install globally, not much we can do about that one unfortunately. I went ahead and added phantomjs as a dependency though. I believe all that remains is updating the readme, which I'll get to tomorrow morning.

Still good for this once that's done?

joefiorini commented 11 years ago

@raimohanska I've added details to the README. For the most part, the workflow hasn't changed since we still have the same NPM scripts that we had before. Does this look good enough? Anything else you'd like to see in there?

raimohanska commented 11 years ago

Hmm.. The browser tests don't work for me.

I did bower install, npm test and got

Running "karma:dev" (karma) task
INFO [karma]: Karma server started at http://localhost:9876/
INFO [launcher]: Starting browser PhantomJS
WARN [watcher]: Pattern "/Users/juha/code/bacon.jquery/bower_components/jquery/jquery.js" does not match any file.
WARN [watcher]: Pattern "/Users/juha/code/bacon.jquery/bower_components/bacon/dist/Bacon.js" does not match any file.
WARN [watcher]: Pattern "/Users/juha/code/bacon.jquery/bower_components/jquery-mockjax/jquery.mockjax.js" does not match any file.

It just gets stuck. Nothing happens. Then after a minute

WARN [launcher]: PhantomJS have not captured in 60000 ms, killing.
INFO [launcher]: Trying to start PhantomJS again.

Unit tests pass when running with grunt test:unit.

In master, the phantom-mocha tests run just fine. I'll try the same on my work laptop tomorrow.

joefiorini commented 11 years ago

That is strange. I'm assuming bower install created the bower_components directory and downloaded the dependencies okay? What version of bower are you running? What OS are you on?

On Mon, Sep 9, 2013 at 2:50 PM, Juha Paananen notifications@github.comwrote:

Hmm.. The browser tests don't work for me.

I did bower install, npm test and got

Running "karma:dev" (karma) task INFO [karma]: Karma server started at http://localhost:9876/ INFO [launcher]: Starting browser PhantomJS WARN [watcher]: Pattern "/Users/juha/code/bacon.jquery/bower_components/jquery/jquery.js" does not match any file. WARN [watcher]: Pattern "/Users/juha/code/bacon.jquery/bower_components/bacon/dist/Bacon.js" does not match any file. WARN [watcher]: Pattern "/Users/juha/code/bacon.jquery/bower_components/jquery-mockjax/jquery.mockjax.js" does not match any file.

It just gets stuck. Nothing happens. Unit tests pass when running with grunt test:unit.

In master, the phantom-mocha tests run just fine. I'll try the same on my work laptop tomorrow.

— Reply to this email directly or view it on GitHubhttps://github.com/baconjs/bacon.jquery/pull/12#issuecomment-24104588 .

joefiorini commented 11 years ago

I just cloned bacon.jquery on my laptop to a new directory. Went in and ran npm install, bower install and then npm test. I had 40 out of 40 tests executed successfully. I also tried it on an Ubuntu cloud server and it works for me there to. Do you have some time this week we could pair up and troubleshoot what's happening?

raimohanska commented 11 years ago

On my other machine at work, I cloned from your branch (git://github.com/joefiorini/bacon.jquery.git), and bower installed and npm installed and got:

Error: Cannot find module 'matchdep'

Then npm install matchdep then npm install and got

>> Local Npm module "grunt-cli" not found. Is it installed?
>> Local Npm module "grunt-es6-module-transpiler" not found. Is it installed?
>> Local Npm module "grunt-simple-mocha" not found. Is it installed?
>> Local Npm module "grunt-karma" not found. Is it installed?
>> Local Npm module "grunt-release" not found. Is it installed?

Running "clean:dist" (clean) task
Warning: Task "transpile" not found. Use --force to continue.

Seems that the devDependencies were not installed. Meh.

raimohanska commented 11 years ago

After removing npm-shrinkwrap.json, the deps are now loaded correctly.

Also, tests are run!

Seems to be OK now. I'll do some reviewing and then merge.

raimohanska commented 11 years ago

I released 0.3.5 using grunt release but the result was foobar, so I reverted this and force pushed 0.3.3 status to master.

More specifically, the created version bacon.jquery@0.3.5 cannot be installed using npm.

So, that needs to be fixed before trying to merge again.

raimohanska commented 11 years ago

Got to admit this is quite frustrating. Not sure whether it's worth the effort.

raimohanska commented 11 years ago

Forget about that ^. We'll fix it :)

raimohanska commented 11 years ago

Still, on my home MBP, npm test gets stuck as before.

raimohanska commented 11 years ago

Updated to node 0.10.16 and got one more different error

PhantomJS 1.9 (Mac) ERROR ReferenceError: Can't find variable: Bacon at /Users/juha/code/bacon-joe/tmp/test/run/bacon.jquery.js:11 PhantomJS 1.9 (Mac): Executed 0 of 0 ERROR (0.03 secs / 0 secs)

raimohanska commented 11 years ago

On the same machine, with updated node, the master branch still works like a Buick (a Finnish saying).

Another saying would be that it works like the toilet on a train.