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

Update Cloak #63

Merged
merged 5 commits into from
Jun 18, 2014
Merged

Update Cloak #63

merged 5 commits into from
Jun 18, 2014

Conversation

jugglinmike
Copy link
Contributor

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 jugglinmike changed the title Update cloak Update Cloak May 10, 2014
@jugglinmike
Copy link
Contributor Author

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

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

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!

The latest release of the Cloak library supports AMD, so the AMD "shim
configuration" can be removed. Because Cloak now depends on a module
with the ID "socket.io-client" and because this path is more accurate
than the current alias ("socket.io"), the CEE codebase should be updated
to comply.
The latest release of the Cloak library does not suffer from the bug in
cleanup logic that prompted this workaround, so it may be safely
removed.
Cloak has since been updated to pass connection options through to
Socket.io, so the library no longer needs to be monkey-patched in this
way.
These dependencies will only be resolved in the context of the running
application. Because the application depends on Lodash, external
dependencies do not need to be built with an additional copy of that
library.
@jugglinmike
Copy link
Contributor Author

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

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

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

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

@stevekinney
Copy link
Member

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
#63 (comment).

@jugglinmike
Copy link
Contributor Author

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

@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. 😠

stevekinney added a commit that referenced this pull request Jun 18, 2014
@stevekinney stevekinney merged commit 5433ad4 into councilforeconed:master Jun 18, 2014
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.

2 participants