search

tsechingho / ckeditor-rails

Integrate CKEditor javascript library with Rails asset pipeline
https://github.com/tsechingho/ckeditor-rails
MIT License
213 stars 134 forks source link

Update to work in Rails 4 non-fingerprinted environment. #21

Closed eric1234 closed 11 years ago

eric1234 commented 11 years ago

We do this by cheating. Instead of do some crazy mojo to get ckeditor talking to the fingerprinted versions of the assets we simply create non-fingerprinted versions like it used to work in Rails 3.

Since this is not what a developer might expect happen we limit our cheat to only assets produced by ckeditor.

Our task that does this is named assets:precompile. This way our functionality is appended to the standard funtionality. This will allow it to work in environments like Heroku where the name of the task that is run to generate the assets cannot be changed.

The end result is that everything should work out-of-the-box, not affect anything bug ckeditor and requires minimal changes to this gem.

This fixes issue #18.

dcabo commented 11 years ago

Hi, I think there's an issue with this fix: I'm trying to override the default ckeditor/config.js and deploy to Heroku (using Rails 4), but it only works in some deploys, in a non-deterministic way (as far as I can see). The problem is that I can see two versions of the fingerprinted config.js being copied: the default one from CKEditor, and my customized one. Part of my Heroku deployment log below:

   Running: rake assets:precompile
   WARNING: Nokogiri was built against LibXML version 2.8.0, but has dynamically loaded 2.7.6
   cp public/assets/ckeditor/build-config-06b782164bd979dac725306b28f2bcd2.js.gz public/assets/ckeditor/build-config.js.gz
   cp public/assets/ckeditor/README-7becffaf31bdb2ef5ca5d5037483a79f.md public/assets/ckeditor/README.md
   => cp public/assets/ckeditor/config-d6da010ddac464fd26b69bbd9afaf7e3.js public/assets/ckeditor/config.js
   cp public/assets/ckeditor/CHANGES-865a4168e76b313041362eac2f3ada88.md public/assets/ckeditor/CHANGES.md
   cp public/assets/ckeditor/contents-0d8ffa186a00f5063461bc0ba0d96087.css.gz public/assets/ckeditor/contents.css.gz
   cp public/assets/ckeditor/ckeditor-4d6f64ec33991b523aad4e21547367ec.js public/assets/ckeditor/ckeditor.js
   cp public/assets/ckeditor/build-config-06b782164bd979dac725306b28f2bcd2.js public/assets/ckeditor/build-config.js
   cp public/assets/ckeditor/LICENSE-306f653a094d1df64024837e824c17b1.md public/assets/ckeditor/LICENSE.md
   cp public/assets/ckeditor/basepath-fced6b200b33dd1223bb7cfd1adf723f.js.gz public/assets/ckeditor/basepath.js.gz
   cp public/assets/ckeditor/jquery-49ec9ba800014b996e8da45df9278418.js.gz public/assets/ckeditor/jquery.js.gz
   => cp public/assets/ckeditor/config-b8c77e48d4334695a489442874857c27.js public/assets/ckeditor/config.js
   cp public/assets/ckeditor/styles-93e62bc976b85c2540555c01cde86759.js.gz public/assets/ckeditor/styles.js.gz

Since only the last version of the file remains, at the moment I can guarantee that my file will be used. :/

eric1234 commented 11 years ago

I think you are correct. I am not taking into account that the asset directory might be dirty with previous compiles.

Instead of blindly copying any fingerprinted asset into a non-fingerprinted version we need to focus on only the latest fingerprint. My guess is that we can use whatever method Rails is using to determine that latest fingerprint (maybe the manifest). If you get a chance to patch go for it. Otherwise I'll take a look in the next day or two to see if I can patch it up.

dcabo commented 11 years ago

Not sure that I understand all the pieces involved here, but the second file is not coming from a previous compile, this is deploying to Heroku. There are two config.js files here: the default/empty one coming from the ckeditor-rails gem, plus the one I created in my Rails app (under app/assets/javascripts/ckeditor) to override some defaults. At the moment it's not guaranteed that my app file will override the gem one.

I'll try to think of a way of fixing this, although as a short-term workaround I'm considering just forking the gem and use that.

dcabo commented 11 years ago

Sorry @eric1234, I think you were right: I assumed Heroku started with a fresh environment on each deploy, but now I see that it's piling up new compilations on top of previous ones. I thought the problem was on the Rails 4 asset pipeline, but no, it's clearly on the Heroku side.

dcabo commented 11 years ago

Ok, this is my current workaround, which seems to be working fine in Heroku: I compare the file's modification time (if the target exists) and copy the file only if the fingerprinted one is newer.

tsechingho commented 11 years ago

@dcabo could you make a pull request to me? It looks good to make more file check. Thanks a lot.

eric1234 commented 11 years ago

@dcabo - It actually is a problem with my patch and not a problem on Heroku. Although Heroku does pile up new compilations this is expected behavior for Rails. This allows overlap between updates so older application servers and newer application servers can each point to valid assets. Also if you are caching you might need older assets. There is an explicit task you can run to clear out older assets when you do not need them anymore.

I would lean away from using the modification time for the file as a way of determining the latest one. Depending on how files are copied around this may or may not be reliable. I think what we actually need to do is read the manifest.yml file which spells out exactly which file is the latest.

dcabo commented 11 years ago

Hi @eric1234, thanks for the explanation, I hadn't thought about this scenario of multiple compilations overlapping each other. I agree that a more robust solution involves parsing the manifest file, but for now I've submitted a pull request that uses just the modification time of the file. It's probably not perfect, but gives you deterministic behaviour, at least in the Heroku case: the precompiled assets of the latest deployment are guaranteed to overwrite the existing ones, which wasn't the case before.