councilforeconed / interactive-activities

Council for Economic Education
http://interactives.councilforeconed.org
Mozilla Public License 2.0
6 stars 2 forks source link

Update Cloak #63

Closed jugglinmike closed 10 years ago

jugglinmike commented 10 years ago

The Cloak library has improved since we started working with it! Some of these improvements constitute breaking changes, but in a slight breech of etiquette, they have been published as a "patch" release. This project uses npm's ^ operator for pessimistic dependency resolution, so a breaking change in a patch release blocks other work.

Although not technically necessary to move forward, I've took advantage of another improvement and removed that pesky "refresh" logic from the network activities. We all hated that.

@mzgoddard What do you think?

jugglinmike commented 10 years ago

Do not merge

This change works in development mode but breaks the build. Because Cloak now has a hard dependency on Underscore, we need to use RequireJS's "map" configuration as we do for the main application build. I'm trying to find a good way to express this without duplication, but it's not as trivial as I would have thought. I'll keep you posted, @mzgoddard .

jugglinmike commented 10 years ago

Still working on this. In the mean time, I've pinned Cloak to version 0.2.3 so that other work (i.e. #58) can proceed.

jugglinmike commented 10 years ago

Okay! I've gotten this building correctly. In the process, I've removed another ugly hack (@mzgoddard: git rm src/client/scripts/socketio-monkey.js), which is good. Unfortunately, there is another bug in Cloak that prevents this from working as-is. I've opened a pull request to fix that, and I'll bug Greg to release a new version so we can land this. Stay tuned!

jugglinmike commented 10 years ago

Greg just published Cloak version 0.2.5 for us! I've rebased this onto master, so it should be good to merge. I think this removes all the rough edges we introduced while integrating with Cloak :)

@stevekinney Would you mind reviewing for me? Since this modifies the build process, you'll also want to test against grunt build && grunt server:hang:prod.

stevekinney commented 10 years ago

@jugglinmike When I use grunt build && grunt server:hang:prod, the server loads up but I just get blank pages (well, blank but with the header and footer from the home page) when I visit either Pizza Productivity or Cacao without having created a room first. When I create a room it just hangs on the loading screen in perpetuity.

When I build run it in development, I get the following errors during the build process:

  15) sync "before each" hook 
     TypeError: Should wrap property of object
    at Object.wrapMethod (http://localhost:7357/2765/node_modules/sinon/lib/sinon.js:63:23)
    at Object.stub (http://localhost:7357/2765/node_modules/sinon/lib/sinon/stub.js:56:22)
    at Context.<anonymous> (http://localhost:7357/2765/test/unit/client/tests/scripts/sync.js:27:13)
    at Hook.Runnable.run (http://localhost:7357/2765/test/unit/client/lib/mocha.js:4336:32)
    at next (http://localhost:7357/2765/test/unit/client/lib/mocha.js:4609:10)
    at http://localhost:7357/2765/test/unit/client/lib/mocha.js:4626:5
    at timeslice (http://localhost:7357/2765/test/unit/client/lib/mocha.js:5733:27)  16) sync "after each" hook 
     TypeError: Cannot read property 'message' of undefined
    at Context.<anonymous> (http://localhost:7357/2765/test/unit/client/tests/scripts/sync.js:31:12)
    at Hook.Runnable.run (http://localhost:7357/2765/test/unit/client/lib/mocha.js:4336:32)
    at next (http://localhost:7357/2765/test/unit/client/lib/mocha.js:4609:10)
    at http://localhost:7357/2765/test/unit/client/lib/mocha.js:4626:5
    at timeslice (http://localhost:7357/2765/test/unit/client/lib/mocha.js:5733:27)  37) sync "before each" hook 
     TypeError: Should wrap property of object
    at Object.wrapMethod (http://localhost:7357/4946/node_modules/sinon/lib/sinon.js:63:23)
    at Object.stub (http://localhost:7357/4946/node_modules/sinon/lib/sinon/stub.js:56:22)
    at Context.<anonymous> (http://localhost:7357/4946/test/unit/client/tests/scripts/sync.js:27:13)
    at Hook.Runnable.run (http://localhost:7357/4946/test/unit/client/lib/mocha.js:4336:32)
    at next (http://localhost:7357/4946/test/unit/client/lib/mocha.js:4609:10)
    at http://localhost:7357/4946/test/unit/client/lib/mocha.js:4626:5
    at timeslice (http://localhost:7357/4946/test/unit/client/lib/mocha.js:5733:27)  38) sync "after each" hook 
     TypeError: Cannot read property 'message' of undefined
    at Context.<anonymous> (http://localhost:7357/4946/test/unit/client/tests/scripts/sync.js:31:12)
    at Hook.Runnable.run (http://localhost:7357/4946/test/unit/client/lib/mocha.js:4336:32)
    at next (http://localhost:7357/4946/test/unit/client/lib/mocha.js:4609:10)
    at http://localhost:7357/4946/test/unit/client/lib/mocha.js:4626:5
    at timeslice (http://localhost:7357/4946/test/unit/client/lib/mocha.js:5733:27)  59) sync "before each" hook 
     wrapMethod@http://localhost:7357/7387/node_modules/sinon/lib/sinon.js:63:70
stub@http://localhost:7357/7387/node_modules/sinon/lib/sinon/stub.js:56:32
http://localhost:7357/7387/test/unit/client/tests/scripts/sync.js:27:17
run@http://localhost:7357/7387/test/unit/client/lib/mocha.js:4336:36
next@http://localhost:7357/7387/test/unit/client/lib/mocha.js:4609:13
http://localhost:7357/7387/test/unit/client/lib/mocha.js:4626:9
timeslice@http://localhost:7357/7387/test/unit/client/lib/mocha.js:5733:27  60) sync "after each" hook 
     http://localhost:7357/7387/test/unit/client/tests/scripts/sync.js:31:12
run@http://localhost:7357/7387/test/unit/client/lib/mocha.js:4336:36
next@http://localhost:7357/7387/test/unit/client/lib/mocha.js:4609:13
http://localhost:7357/7387/test/unit/client/lib/mocha.js:4626:9
timeslice@http://localhost:7357/7387/test/unit/client/lib/mocha.js:5733:27Warning: Task "testem:ci:client" failed. Use --force to continue.
stevekinney commented 10 years ago

@jugglinmike Let me know if you get a chance to poke at this. Maybe it's just my machine, but it would be awesome if someone could confirm that before I merge it.

jugglinmike commented 10 years ago

@stevekinney Totally! I was out of town last week, but I should have some time to investigate within the next two days.

stevekinney commented 10 years ago

You are a gentleman and a scholar.

On Mon, Jun 16, 2014 at 10:42 AM, jugglinmike notifications@github.com wrote:

@stevekinney https://github.com/stevekinney Totally! I was out of town last week, but I should have some time to investigate within the next two days.

— Reply to this email directly or view it on GitHub https://github.com/councilforeconed/cee/pull/63#issuecomment-46187047.

jugglinmike commented 10 years ago

Hey Steve,

I'm unable to reproduce the problems you've described. I've tried to do this in as clean an environment as possible (short of booting a virtual machine). Here are my steps:

$ npm cache clean
$ bower cache clean
$ git clone git@github.com:jugglinmike/cee.git tmp
$ cd tmp
$ git checkout update-cloak
$ npm install
$ grunt test
$ grunt build
$ grunt server:hang:prod

...the tests passed, the build completed without issue, and I was able to visit the server running on localhost, load the Cacao activity without a room, create a room for Cacao, and successfully complete a transaction between two "players" across tabs.

Then I thought that maybe a bug is being introduced when my patch is applied to the latest version of master on CEE's repo. I ran the following steps to test that theory:

$ npm cache clean
$ bower cache clean
$ git clone git@github.com:councilforeconed/cee.git tmp
$ cd tmp
$ git pull git@github.com:jugglinmike/cee.git update-cloak
$ npm install
$ grunt test --force
$ grunt build
$ grunt server:hang:prod

In this case, I needed to use the --force flag when running the test task in order to pass lint. Other than that, though, I was able to complete the same steps described above.

I'm a little stumped here. Can you think of anything I'm missing? Maybe we have some incompatability in our globally-installed dependencies:

$ node --version
v0.10.26
$ npm --version
1.4.3
$ grunt --version
grunt-cli v0.1.13
grunt v0.4.5
$ bower --version
1.3.5

...although that seems like a stretch.

stevekinney commented 10 years ago

@jugglinmike, Following those steps seems to work. My laptop was a bit behind the times (v0.10.21), I'm not sure if that was the reason, but something in that recipe did the trick. So, I'm going to go ahead and merge it.

Now, I just need to have a word with the jerk (me) who left all those linting errors in the money creation chart. :angry: