rbCAS / CASino

CASino is a Ruby-based Single Sign-On solution supporting the CAS standard
MIT License
331 stars 189 forks source link

Removes jQuery dependency #27

Closed dlindahl closed 11 years ago

dlindahl commented 11 years ago

jQuery Rails is now at v3, so forcing a lower version means that some users will run into dependency conflicts when installing CASino (especially Rails 4 users)

Additionally, not only do some users not use (or want) jQuery in their application, CASino isn't using that much javascript to necessitate pulling in all of jQuery.

This updates the javascript in two ways:

coveralls commented 11 years ago

Coverage Status

Coverage remained the same when pulling bde350d1923e0fcd84441c19218658a1012eadef on dlindahl:features/remove_jquery_dependency into 50be8f059f2c4f3e7f59b8da6e694bcad1b0a0b3 on rbCAS:release/2.0.

pencil commented 11 years ago

Nice! But Travis has errors when running the specs. Could you please fix this?

dlindahl commented 11 years ago

d'oh. My bad, will fix in a few

pencil commented 11 years ago

Cool, thanks. Please squash afterwards, I prefer not to merge failing commits :smiley:

coveralls commented 11 years ago

Coverage Status

Coverage remained the same when pulling d6cbe59e0e2abda72fcf5e97f3a9b3cb1ce27461 on dlindahl:features/remove_jquery_dependency into a25fa5721aeb8733df1938b176c64612e428373d on rbCAS:release/2.0.

dlindahl commented 11 years ago

Fixed the failing test by removing the dependency on CoffeeScript.

The tests were failing because sessions.js.coffee is inlined to ensure that it is always run on the page and coffee-script isn't required in test mode. Since having an implicit dependency on CoffeeScript is less than ideal anyway, I just rewrote sessions.js.coffee to use plain old JavaScript, thus avoiding the problem.

pencil commented 11 years ago

Is it a good idea to remove CoffeeScript? What would be the steps to add it as an explicit dependency? I'm planning to add some more JavaScript to the Session Overview page and would prefer to write it in CoffeeScript.

pencil commented 11 years ago

Noticed during testing in our staging environment that the logic was quite a bit off. Furthermore the inline script magic did not work there. Probably because it was precompiled.

Fixed both issues and released CASino 2.0.0 since all backward compatibility breaking PRs were merged. :smile:

dlindahl commented 11 years ago

The issue of CoffeeScript can get pretty divisive and in some circumstances, its enough to cause people to not use a particular project. Given the amount of javascript in this project, it was a pretty easy decision in my opinion.

If you plan on adding more, I would still consider using plain old Javascript. If you really must use CoffeeScript, then you might want to consider adding something like Grunt as a development dependency to manually compile the CoffeeScript into assets/javascripts. That way you can internally write in CoffeeScript, but you aren't adding coffeescript as a gem dependency. Only you guys can make the call if its worth it or not. Besides, Javascript isn't all that bad :wink:

My mistake on the munging of auto-login code. I tested it manually as best as I could but must have misunderstood the intention of the code.

As far as the inlining goes, it is likely due to the pre-compilation step in production as you said. I have used something similar in other apps so it should be a sound solution. I must have missed something in the implementation though. Once I get to that point in my own app, I'll re-implement and resubmit a PR.