typescript-ruby / typescript-rails

An asset pipeline wrapper for the TypeScript language
MIT License
255 stars 30 forks source link

Updated with two fixes for Rails 4 and Ruby 2.0. #11

Closed pdxmholmes closed 9 years ago

pdxmholmes commented 10 years ago

Just some quick tweaks for Rails 4/Ruby 2.0:

Added a check to the sprocket initialization to load if Rails >= 4 Updated the references Regex to play nice with Ruby 2.0 regex and fixed a minor bug causing errors when more than one reference line is provided.

pdxmholmes commented 10 years ago

Oh, I should explain why I changed the gsub call to the block form: There was an odd 'bug' (feature?) in Ruby 2.0 where when using the string form ("\1 blah \2 \3"), the last directory separator character of escaped_path was getting chopped off during the File.join. Changing to the block version seen here fixed it, is backwards compatibility, and shouldn't incur any sort of performance hit.

I confirmed this on Windows, Linux and OS X, latest build of Ruby 2.0.

klaustopher commented 10 years ago

Looks good. I'll incluse this and also bump the typescript version and release a new version of the gem

pdxmholmes commented 10 years ago

Oh I already bumped TypeScript to 0.9.0.1 and sent a pullrequest to tkawachi.

https://github.com/brainling/typescript-src-ruby

tkawachi commented 10 years ago

Thanks @brainling, I merged & released it.

klaustopher commented 10 years ago

@brainling Can you take a look at https://travis-ci.org/klaustopher/typescript-rails/builds/9380644 and check why the build is failing?

pdxmholmes commented 10 years ago

I'll take a peek when I get home from work. May be a Ruby 2.0/1.9 thing? I'll try it on Ruby 1.9 on my Mac where I have RVM and it's easier to change over.

pdxmholmes commented 10 years ago

Okay, I just pushed a set of changes that should fix it:

I re-write the reference algorithm to not use gsub because of some variable scoping oddness in Ruby 2.0. The code is a tiny bit less clean but it works.

I also had to tweak some of the tests and test files, as some of they were not 0.9 compatible.

I was able to run all tests in the context of Rails 3 and Rails 4, and all pass now.

klaustopher commented 10 years ago

It is still failing on travis :frowning:

pdxmholmes commented 10 years ago

It's a line ending issue. I was thinking last night that embedding the line endings in the result checks like that is brittle, but I'm not sure what to do about it? On both my machines (OS X and Windows), it's outputting simple '\n'...yet it seems on Travis it's doing \r\n? The line endings can't simply be copied from the .ts files, because they are different on my two machines (I have Git automatically flip line endings on pull/commit)...so I'm really not sure what to do about it? Is there some better way we could handle the line endings in the result checking? I guess we could use regex instead of just embedding the line endings like that.

klaustopher commented 10 years ago

We could either use \s+ in the regex or use/set the global line seperator $/

pdxmholmes commented 10 years ago

They are passing here on my work machine, so let me play with the regex and see if I can get them to pass on travis.

pdxmholmes commented 10 years ago

Okay, Travis is now passing for MRI 1.9/2.0. It's failing for the unofficial Rubies (RBX, REE, JRuby), but the failure is deep in typescript-node, as evidenced by this message from the JRuby build:

ActionView::Template::Error: undefined method value' for nil:NilClass /home/travis/.rvm/gems/jruby-1.7.4-d19/gems/typescript-node-0.0.5/lib/typescript-node.rb:21:incompile_file' org/jruby/RubyIO.java:4093:in `popen3'

pdxmholmes commented 10 years ago

Alright, one last set of changes. I added a unit test for multiple references, and that turned up a bug, which I fixed. It's green on Travis across all the MRI's.

klaustopher commented 10 years ago

@brainling Sorry for not getting back to you, but a lot of stuff has been going on. Would you maybe want to take over development?

pdxmholmes commented 10 years ago

Hmmm. Let me think about it. Sorry for my late reply as well.

I'm about to move, and after that I may have more free time, so it's something I'll consider.